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 <jpe...@apache.org> wrote: > > > >> > >> On Nov 29, 2016, at 11:38 AM, Meera Mosale Nataraja <mech...@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. > >
