[jira] [Commented] (TS-2584) failed assert transforming and caching negative response

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/TS-2584?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13958154#comment-13958154 ] 

ASF GitHub Bot commented on TS-2584:
------------------------------------

Github user jpeach commented on the pull request:

    https://github.com/apache/trafficserver/pull/44#issuecomment-39381147
  
    I'm not confident that this change is correct. The root cause of the assertion seems to be that `cache_info.transform_store` is not handled the same way as `cache_info.object_store`. In `HttpTransact::handle_transform_ready`, we know that the `transform_response` is valid, so that might be a reasonable place to  set the response into `cache_info.transform_store`.
    
    I would also like to understand why `HttpTransact::set_headers_for_cache_write` cares about caching a negative response at all. I *think* that this is all about setting up a special response to write to cache when caching a negative response and being careful not to trash that with a different response header.
    
    Finally, it sounds like it should be simple to write a test for this in the `ci/tsqa` harness using the null transform example plugin and curl.


> failed assert transforming and caching negative response
> --------------------------------------------------------
>
>                 Key: TS-2584
>                 URL: https://issues.apache.org/jira/browse/TS-2584
>             Project: Traffic Server
>          Issue Type: Bug
>          Components: HTTP
>            Reporter: Jack Bates
>              Labels: Review
>             Fix For: 5.0.0
>
>
> If negative caching is enabled and you transform a negative response (for example a null transform) there is a failed assert in HttpTransact::set_headers_for_cache_write()
> {noformat}
> FATAL: HttpTransact.cc:4811: failed assert `cache_info->response_get()->valid()`
> proxy/.libs/lt-traffic_server - STACK TRACE:
> lib/ts/.libs/libtsutil.so.5(ink_fatal_die+0x0)[0xb77a6445]
> lib/ts/.libs/libtsutil.so.5(+0x22269)[0xb77a5269]
> proxy/.libs/lt-traffic_server(_ZN12HttpTransact27set_headers_for_cache_writeEPNS_5StateEP8HTTPInfoP7HTTPHdrS5_+0x1da)[0x81c0ae6]
> proxy/.libs/lt-traffic_server(_ZN12HttpTransact22handle_transform_readyEPNS_5StateE+0x272)[0x81c0576]
> proxy/.libs/lt-traffic_server(_ZN6HttpSM32call_transact_and_set_next_stateEPFvPN12HttpTransact5StateEE+0x78)[0x81a4b3e]
> proxy/.libs/lt-traffic_server(_ZN6HttpSM38state_response_wait_for_transform_readEiPv+0x196)[0x81926b0]
> proxy/.libs/lt-traffic_server(_ZN6HttpSM12main_handlerEiPv+0x2df)[0x81969b5]
> proxy/.libs/lt-traffic_server(_ZN12Continuation11handleEventEiPv+0x47)[0x811b427]
> proxy/.libs/lt-traffic_server(_ZN17TransformTerminus12handle_eventEiPv+0x27f)[0x815d81b]
> proxy/.libs/lt-traffic_server(_ZN12Continuation11handleEventEiPv+0x47)[0x811b427]
> proxy/.libs/lt-traffic_server(_ZN7EThread13process_eventEP5Eventi+0x104)[0x8300692]
> proxy/.libs/lt-traffic_server(_ZN7EThread7executeEv+0xd6)[0x8300916]
> proxy/.libs/lt-traffic_server[0x82ff569]
> /lib/i686/cmov/libpthread.so.0(+0x5955)[0xb7467955]
> /lib/i686/cmov/libc.so.6(clone+0x5e)[0xb717f1de]
> Aborted (core dumped)
> {noformat}
> HttpTransact::handle_transform_ready() passes s->cache_info.transform_store to HttpTransact::set_headers_for_cache_write()
> The ->valid() test at the top of HttpTransact::set_headers_for_cache_write() fails so ->create() gets called.
> {code}
>   if (!cache_info->valid()) {
>     cache_info->create();
>   }
> {code}
> (s->cache_info.transform_store->m_alt is NULL. I didn't look into why this is, is it expected?)
> Because s->negative_caching is true, cache_info->response_set(response) doesn't get called, instead the assert fails.
> {code}
>   if (!s->negative_caching)
>     cache_info->response_set(response);
>   else {
>     ink_assert(cache_info->response_get()->valid());
>   }
> {code}
> Assuming the cache_info->valid() test can fail and s->negative_caching can be true at the same time, one possible solution is to fix the logic so cache_info->response_set(response) gets called?



--
This message was sent by Atlassian JIRA
(v6.2#6252)