You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by Leif Hedstrom <zw...@apache.org> on 2016/12/01 18:34:42 UTC
Re: Multiple times gzip issue and TSHttpTxnHookAdd API
> On Nov 29, 2016, at 7:57 PM, James Peach <jp...@apache.org> wrote:
>
>>
>> On Nov 29, 2016, at 11:38 AM, Meera Mosale Nataraja <me...@gmail.com> wrote:
>>
>> Hello all,
>>
>> I'm working on TS-5024 <https://issues.apache.org/jira/browse/TS-5024> where
>> the content is gzip’ed multiple times. Please find the sample output
>> provided below which indicates multiple gzips.
>>
>> curl -v -o/dev/null http://proxy-test:8080/get -H "Host: proxy-test" -x
>> localhost:8080 -H "Accept-encoding: gzip"
>>
>> - About to connect() to proxy localhost port 8080 (#0)
>> - Trying ::1... connected
>> - Connected to localhost (::1) port 8080 (#0)
>>> GET http://proxy-test:8080/get HTTP/1.1
>>> User-Agent: curl/7.19.7 (x86_64-redhat-linux-gnu) libcurl/7.19.7
>> NSS/3.21 Basic ECC zlib/1.2.3 libidn/1.18 libssh2/1.4.2
>>> Accept: */*
>>> Proxy-Connection: Keep-Alive
>>> Host: proxy-test
>>> Accept-encoding: gzip
>>>
>> % Total % Received % Xferd Average Speed Time Time Time Current
>> Dload Upload Total Spent Left Speed
>> 0 0 0 0 0 0 0 0 -::- -::- -::- 0< HTTP/1.1 404 Not Found
>> < Server: ATS/7.1.0
>> < X-Frame-Options: SAMEORIGIN
>> < X-Xss-Protection: 1; mode=block
>> < Accept-Ranges: bytes
>> < X-Content-Type-Options: nosniff
>> < Content-Type: text/html; charset=UTF-8
>> < Cache-Control: max-age=300
>> < Expires: Mon, 31 Oct 2016 18:29:44 GMT
>> < Date: Mon, 31 Oct 2016 18:24:44 GMT
>> < Content-Encoding: gzip
>> < Vary: Accept-Encoding
>> < Content-Encoding: gzip
>> < Content-Encoding: gzip
>> < Content-Length: 4456
>> < Age: 0
>> < Proxy-Connection: keep-alive
>>
>>
>> For example, if OS returns 302 and redirection is enabled, send request
>> hook is called multiple times and the plugin then registers the
>> TS_HTTP_READ_RESPONSE_HDR_HOOK multiple times - once for the first request
>> and again for the redirected request. Hence TSHttpTxnHookAdd API adds the
>> hook multiple times without checking if the hook is already present in the
>> list of hooks. This will result in multiple transformations and each of
>> them trying to gzip the content.
>>
>> One solution is to modify the current implementation of TSHttpTxnHookAdd by
>> adding the functionality to traverse the list of hooks and append the hook
>> only if it is not present in the list. Please find the changeset provided
>> below.
>
> This isn’t sufficient when the plugin attaches a new continuation to the hook, eg. esi, pagespeed, cache_range_requests, remap_purge, etc.
Right, this was pointe out. It covers many, but not all, cases. Fixing this for all cases requires noticeably more code, something that I think Alan is also working on.
>
> I don’t think this would be enough for gzip either, since it creates a nee transform continuation each time.
I think it is sufficient (Meera: you tested this, right?). The issue is that the hook list is retained when the HttpSM restarts, and then we add the same continuation (same contp) multiple times.
The big questions here though are:
1) Can / should we treat TSHttpTxnHookAdd() as idempotent in general (by design)?
2) Can we “break” TSHttpTxnHookAdd() in mid-release?
I’d argue that the answer is “yes” to both (I like to see this as a fundamental bug in TSHttpTxnHookAdd() :-).
Now, while typing this, and thinking about it some more, another option *might* be that when a continuation is executed, we remove it from the Hook list. Meera / VJ / amc: What do you think of that solution? That *might* break some plugins though, unless they re-register accordingly, but it would solve the gzip plugin.
Cheers,
— Leif
>
>>
>> diff --git proxy/InkAPI.cc <http://inkapi.cc/> proxy/InkAPI.cc
>> <http://inkapi.cc/>
>> index 48697d5..8b73779 100644
>> --- proxy/InkAPI.cc <http://inkapi.cc/>
>> +++ proxy/InkAPI.cc <http://inkapi.cc/>
>> @@ -4599,6 +4599,15 @@ TSHttpTxnHookAdd(TSHttpTxn txnp, TSHttpHookID id,
>> TSCont contp)
>> sdk_assert(sdk_sanity_check_hook_id(id) == TS_SUCCESS);
>>
>> HttpSM *sm = (HttpSM *)txnp;
>> + APIHook *hook = sm->txn_hook_get(id);
>> +
>> + // Traverse list of hooks and add a particular hook only once
>> + while (hook != NULL) {
>> + if (hook->m_cont == (INKContInternal *)contp) {
>> + return;
>> + }
>> + hook = hook->m_link.next;
>> + }
>> sm->txn_hook_append(id, (INKContInternal *)contp);
>> }
>>
>> If we think a plugin would need a functionality of calling the hook
>> multiple times we can add a new API but i can’t think of any plugin that
>> would need such an API. Please let me know your thoughts and feedback.
>>
>> Meera.
Re: Multiple times gzip issue and TSHttpTxnHookAdd API
Posted by Meera Mosale Nataraja <me...@gmail.com>.
On Thu, Dec 1, 2016 at 10:34 AM, Leif Hedstrom <zw...@apache.org> wrote:
>
> > On Nov 29, 2016, at 7:57 PM, James Peach <jp...@apache.org> wrote:
> >
> >>
> >> On Nov 29, 2016, at 11:38 AM, Meera Mosale Nataraja <me...@gmail.com>
> wrote:
> >>
> >> Hello all,
> >>
> >> I'm working on TS-5024 <https://issues.apache.org/jira/browse/TS-5024>
> where
> >> the content is gzip’ed multiple times. Please find the sample output
> >> provided below which indicates multiple gzips.
> >>
> >> curl -v -o/dev/null http://proxy-test:8080/get -H "Host: proxy-test" -x
> >> localhost:8080 -H "Accept-encoding: gzip"
> >>
> >> - About to connect() to proxy localhost port 8080 (#0)
> >> - Trying ::1... connected
> >> - Connected to localhost (::1) port 8080 (#0)
> >>> GET http://proxy-test:8080/get HTTP/1.1
> >>> User-Agent: curl/7.19.7 (x86_64-redhat-linux-gnu) libcurl/7.19.7
> >> NSS/3.21 Basic ECC zlib/1.2.3 libidn/1.18 libssh2/1.4.2
> >>> Accept: */*
> >>> Proxy-Connection: Keep-Alive
> >>> Host: proxy-test
> >>> Accept-encoding: gzip
> >>>
> >> % Total % Received % Xferd Average Speed Time Time Time Current
> >> Dload Upload Total Spent Left Speed
> >> 0 0 0 0 0 0 0 0 -::- -::- -::- 0< HTTP/1.1 404 Not Found
> >> < Server: ATS/7.1.0
> >> < X-Frame-Options: SAMEORIGIN
> >> < X-Xss-Protection: 1; mode=block
> >> < Accept-Ranges: bytes
> >> < X-Content-Type-Options: nosniff
> >> < Content-Type: text/html; charset=UTF-8
> >> < Cache-Control: max-age=300
> >> < Expires: Mon, 31 Oct 2016 18:29:44 GMT
> >> < Date: Mon, 31 Oct 2016 18:24:44 GMT
> >> < Content-Encoding: gzip
> >> < Vary: Accept-Encoding
> >> < Content-Encoding: gzip
> >> < Content-Encoding: gzip
> >> < Content-Length: 4456
> >> < Age: 0
> >> < Proxy-Connection: keep-alive
> >>
> >>
> >> For example, if OS returns 302 and redirection is enabled, send request
> >> hook is called multiple times and the plugin then registers the
> >> TS_HTTP_READ_RESPONSE_HDR_HOOK multiple times - once for the first
> request
> >> and again for the redirected request. Hence TSHttpTxnHookAdd API adds
> the
> >> hook multiple times without checking if the hook is already present in
> the
> >> list of hooks. This will result in multiple transformations and each of
> >> them trying to gzip the content.
> >>
> >> One solution is to modify the current implementation of
> TSHttpTxnHookAdd by
> >> adding the functionality to traverse the list of hooks and append the
> hook
> >> only if it is not present in the list. Please find the changeset
> provided
> >> below.
> >
> > This isn’t sufficient when the plugin attaches a new continuation to the
> hook, eg. esi, pagespeed, cache_range_requests, remap_purge, etc.
>
> Right, this was pointe out. It covers many, but not all, cases. Fixing
> this for all cases requires noticeably more code, something that I think
> Alan is also working on.
>
> >
> > I don’t think this would be enough for gzip either, since it creates a
> nee transform continuation each time.
>
> I think it is sufficient (Meera: you tested this, right?). The issue is
> that the hook list is retained when the HttpSM restarts, and then we add
> the same continuation (same contp) multiple times.
Yes, I have tested it and it works well.
>
> The big questions here though are:
>
> 1) Can / should we treat TSHttpTxnHookAdd() as idempotent in general (by
> design)?
>
> 2) Can we “break” TSHttpTxnHookAdd() in mid-release?
>
>
> I’d argue that the answer is “yes” to both (I like to see this as a
> fundamental bug in TSHttpTxnHookAdd() :-).
>
> Now, while typing this, and thinking about it some more, another option
> *might* be that when a continuation is executed, we remove it from the Hook
> list. Meera / VJ / amc: What do you think of that solution? That *might*
> break some plugins though, unless they re-register accordingly, but it
> would solve the gzip plugin.
>
I will look into this approach as well.
>
> Cheers,
>
> — Leif
>
> >
> >>
> >> diff --git proxy/InkAPI.cc <http://inkapi.cc/> proxy/InkAPI.cc
> >> <http://inkapi.cc/>
> >> index 48697d5..8b73779 100644
> >> --- proxy/InkAPI.cc <http://inkapi.cc/>
> >> +++ proxy/InkAPI.cc <http://inkapi.cc/>
> >> @@ -4599,6 +4599,15 @@ TSHttpTxnHookAdd(TSHttpTxn txnp, TSHttpHookID id,
> >> TSCont contp)
> >> sdk_assert(sdk_sanity_check_hook_id(id) == TS_SUCCESS);
> >>
> >> HttpSM *sm = (HttpSM *)txnp;
> >> + APIHook *hook = sm->txn_hook_get(id);
> >> +
> >> + // Traverse list of hooks and add a particular hook only once
> >> + while (hook != NULL) {
> >> + if (hook->m_cont == (INKContInternal *)contp) {
> >> + return;
> >> + }
> >> + hook = hook->m_link.next;
> >> + }
> >> sm->txn_hook_append(id, (INKContInternal *)contp);
> >> }
> >>
> >> If we think a plugin would need a functionality of calling the hook
> >> multiple times we can add a new API but i can’t think of any plugin that
> >> would need such an API. Please let me know your thoughts and feedback.
> >>
> >> Meera.
>
>