Details

    • Bug
    • Status: Patch Available
    • Major
    • Resolution: Unresolved
    • 3.3.0
    • sometime
    • Plugins
    • None

    Description

      ESI plugin will pass on almost all request headers from user request to all async requests that it tried to make for each of the ESI includes.

      We should not be passing the Connection keep-alive header because we don't want these requests to be "keep-alive" after they are done.

      Attachments

        1. ts-1499.diff
          2 kB
          Shu Kit Francis Chan

        Issue Links

          Activity

            zwoop Leif Hedstrom added a comment -

            Hmmm, why is that? Is this possibly related to the problems (I think?) with FetchSM (fetchpages) not doing Keep-alive?

            zwoop Leif Hedstrom added a comment - Hmmm, why is that? Is this possibly related to the problems (I think?) with FetchSM (fetchpages) not doing Keep-alive?

            Here is a patch that should fix this.

            kichan Shu Kit Francis Chan added a comment - Here is a patch that should fix this.

            My debugging shows that the async request from ATS to origin server for the ESI include is kept alive for "proxy.config.http.keep_alive_no_activity_timeout_in" seconds before returning with a proper "success" fetch event.

            Btw, my patch is simply just not allowing keep-alive in the async request.

            kichan Shu Kit Francis Chan added a comment - My debugging shows that the async request from ATS to origin server for the ESI include is kept alive for "proxy.config.http.keep_alive_no_activity_timeout_in" seconds before returning with a proper "success" fetch event. Btw, my patch is simply just not allowing keep-alive in the async request.
            psudaemon Phil Sorber added a comment -

            I have seen similar behavior with the timeout when the IOBuffer is not properly drained. Dunno if that is the issue here as I haven't had a chance to look at the code in question.

            psudaemon Phil Sorber added a comment - I have seen similar behavior with the timeout when the IOBuffer is not properly drained. Dunno if that is the issue here as I haven't had a chance to look at the code in question.
            zwoop Leif Hedstrom added a comment -

            I think it'd be worthwhile understanding why we can't support Keep-Alive with the ESI plugin. It seems like it would be useful to support it, no?

            zwoop Leif Hedstrom added a comment - I think it'd be worthwhile understanding why we can't support Keep-Alive with the ESI plugin. It seems like it would be useful to support it, no?

            Agreed. I think that the keep-alive was handled properly between user and ATS, and between ATS and origin server.
            The root problem is that ESI plugin uses TSFetchUrl() with the request having the keep-alive connection header and it looks like the success fetch event is not sent till the keep-alive connection timed out. And so the plugin never got involved again to handle the success fetch event until really late.

            The patch is rather just a dumb approach (quick fix, too) to not use keep-alive for these ESI include requests.

            kichan Shu Kit Francis Chan added a comment - Agreed. I think that the keep-alive was handled properly between user and ATS, and between ATS and origin server. The root problem is that ESI plugin uses TSFetchUrl() with the request having the keep-alive connection header and it looks like the success fetch event is not sent till the keep-alive connection timed out. And so the plugin never got involved again to handle the success fetch event until really late. The patch is rather just a dumb approach (quick fix, too) to not use keep-alive for these ESI include requests.
            jamespeach James Peach added a comment -

            Yeh I went through this some time ago and found that FetchSM needs to do it's own request completion detection by examining Content-Length, but that doing so breaks the ABI. I forget what happens with chunked encoding, but it would not surprise me if that also caused problems.

            jamespeach James Peach added a comment - Yeh I went through this some time ago and found that FetchSM needs to do it's own request completion detection by examining Content-Length, but that doing so breaks the ABI. I forget what happens with chunked encoding, but it would not surprise me if that also caused problems.
            zwoop Leif Hedstrom added a comment - Moving to 5.0.0 as per https://cwiki.apache.org/confluence/display/TS/New+Release+Processes
            bcall Bryan Call added a comment -

            Shu Kit Francis Chan Is this still an issue?

            bcall Bryan Call added a comment - Shu Kit Francis Chan Is this still an issue?

            will check it out next week.

            kichan Shu Kit Francis Chan added a comment - will check it out next week.

            People

              kichan Shu Kit Francis Chan
              kichan Shu Kit Francis Chan

              Dates

                Created:
                Updated:

                Slack

                  Issue deployment