Uploaded image for project: 'Subversion'
  1. Subversion
  2. SVN-3979

chunked transfer encoding not always supported

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: ---
    • Fix Version/s: 1.8.0
    • Component/s: libsvn_ra_serf
    • Labels:
      None

      Description

      Serf uses chunked transfer encoding and some HTTP proxies don't support it. 
      Squid is known to be a problem, lighthttpd may be a problem.
      

        Activity

        Hide
        philipm Philip Martin added a comment -

        Is this release critical? The user may not be control the proxy but switching to
        neon is a workaround. So it's OK to make serf the default?
        

        Show
        philipm Philip Martin added a comment - Is this release critical? The user may not be control the proxy but switching to neon is a workaround. So it's OK to make serf the default?
        Hide
        gstein Greg Stein added a comment -

        The short answer is that serf communicates using HTTP/1.1 (defined by RFC 2616, back in June 1999).
        
        With some work, we could support HTTP/1.0. The biggest problem here would be needing to pre-
        compute the size of the request (and use Content-Length: rather than chunked requests), which could 
        result in using a temporary file on disk in some cases. Where we know the request is size-bounded, 
        then we could use a memory buffer. To get really spiffy, the new "memory buffer, then spill to disk" 
        functionality (developed for issue 3888) could be used to avoid the disk in some (many?) request 
        scenarios.
        
        (that buffer/spill code is destined for libsvn_subr in the trunk line of development, for use on the 
        server-side, per a request from cmpilato)
        
        

        Show
        gstein Greg Stein added a comment - The short answer is that serf communicates using HTTP/1.1 (defined by RFC 2616, back in June 1999). With some work, we could support HTTP/1.0. The biggest problem here would be needing to pre- compute the size of the request (and use Content-Length: rather than chunked requests), which could result in using a temporary file on disk in some cases. Where we know the request is size-bounded, then we could use a memory buffer. To get really spiffy, the new "memory buffer, then spill to disk" functionality (developed for issue 3888) could be used to avoid the disk in some (many?) request scenarios. (that buffer/spill code is destined for libsvn_subr in the trunk line of development, for use on the server-side, per a request from cmpilato)
        Hide
        gstein Greg Stein added a comment -

        To be a little clearer on options, and our probably "ideal" design:
        
        The initial OPTIONS request to the server could use HTTP/1.1 with a C-L header. If the server responded 
        with HTTP/1.0, then we could force all future requests down to the 1.0 protocol.
        
        However, it is also possible for the server to respond with a 1.1 request, yet be broken and not support 
        chunked requests. Some sort of per-server override would then be advisable to force the client down to 
        the 1.0 protocol.
        
        (use of 1.0 has other disadvantages such as lack of gzip/deflate; I'm not sure (right now) which features 
        we'd lose; for a broken 1.1 server where we force a 1.0-style request (ie. no chunking) it may even be 
        possible to continue to use those 1.1 features; this is a slippery slope, obviously... we'd probably just 
        want to make it binary and lose all 1.1 features for broken servers rather than trying to mix/match)
        
        And last remark: the svn server *is* an HTTP/1.1 server (Apache httpd). We are only talking about 
        interposed proxy support here.
        
        

        Show
        gstein Greg Stein added a comment - To be a little clearer on options, and our probably "ideal" design: The initial OPTIONS request to the server could use HTTP/1.1 with a C-L header. If the server responded with HTTP/1.0, then we could force all future requests down to the 1.0 protocol. However, it is also possible for the server to respond with a 1.1 request, yet be broken and not support chunked requests. Some sort of per-server override would then be advisable to force the client down to the 1.0 protocol. (use of 1.0 has other disadvantages such as lack of gzip/deflate; I'm not sure (right now) which features we'd lose; for a broken 1.1 server where we force a 1.0-style request (ie. no chunking) it may even be possible to continue to use those 1.1 features; this is a slippery slope, obviously... we'd probably just want to make it binary and lose all 1.1 features for broken servers rather than trying to mix/match) And last remark: the svn server *is* an HTTP/1.1 server (Apache httpd). We are only talking about interposed proxy support here.
        Hide
        philipm Philip Martin added a comment -

        http://mail-archives.apache.org/mod_mbox/subversion-dev/201105.mbox/%3CBANLkTintSFwkzXd6iWsCzXRTL4p27RdkBg@mail.gmail.com%3E
        
        Justin's patch to have serf send content-length.
        

        Show
        philipm Philip Martin added a comment - http://mail-archives.apache.org/mod_mbox/subversion-dev/201105.mbox/%3CBANLkTintSFwkzXd6iWsCzXRTL4p27RdkBg@mail.gmail.com%3E Justin's patch to have serf send content-length.
        Hide
        philipm Philip Martin added a comment -

        Justin's patch is for serf itself, not Subversion/ra_serf.
        

        Show
        philipm Philip Martin added a comment - Justin's patch is for serf itself, not Subversion/ra_serf.
        Hide
        rhuijben Bert Huijben added a comment -

        The nginx reverse proxy which is in use by at least one commercial subversion 
        hoster has a similar (or possibly the same) problem. It returns HTTP error 411 
        Length required on most serf requests I throw at it.
        
        This looks like an easy to diagnose error code for a fallback where we 
        calculate the length.
        
        How does squid report its error?
        

        Show
        rhuijben Bert Huijben added a comment - The nginx reverse proxy which is in use by at least one commercial subversion hoster has a similar (or possibly the same) problem. It returns HTTP error 411 Length required on most serf requests I throw at it. This looks like an easy to diagnose error code for a fallback where we calculate the length. How does squid report its error?
        Hide
        philipm Philip Martin added a comment -

        Squid 3.1.6 gives a 501 Not implemented:
        
        svn: E020014: Unable to connect to a repository at URL
        'http://localhost:8888/obj/repo'
        ../src/subversion/libsvn_ra_serf/util.c:768: (apr_err=20014)
        ../src/subversion/libsvn_ra_serf/util.c:2025: (apr_err=20014)
        ../src/subversion/libsvn_ra_serf/util.c:2025: (apr_err=20014)
        svn: E020014: Unspecified error message: 501 Not Implemented
        
        

        Show
        philipm Philip Martin added a comment - Squid 3.1.6 gives a 501 Not implemented: svn: E020014: Unable to connect to a repository at URL 'http://localhost:8888/obj/repo' ../src/subversion/libsvn_ra_serf/util.c:768: (apr_err=20014) ../src/subversion/libsvn_ra_serf/util.c:2025: (apr_err=20014) ../src/subversion/libsvn_ra_serf/util.c:2025: (apr_err=20014) svn: E020014: Unspecified error message: 501 Not Implemented
        Hide
        gstein Greg Stein added a comment -

        Shift to 1.8.0, after ungating per r1158522.
        

        Show
        gstein Greg Stein added a comment - Shift to 1.8.0, after ungating per r1158522.
        Hide
        philipm Philip Martin added a comment -

        socat appears to be affected as well.
        
        Running something like:
        
          socat -v TCP-LISTEN:9630,reuseaddr,fork TCP:localhost:8080
        
        puts a socat proxy on port 9630 for apache on port 8080. This works when using
        neon but fails when using serf:
        
        ../src/subversion/svn/checkout-cmd.c:168: (apr_err=111)
        ../src/subversion/libsvn_client/checkout.c:137: (apr_err=111)
        ../src/subversion/libsvn_client/ra.c:476: (apr_err=111)
        ../src/subversion/libsvn_client/ra.c:336: (apr_err=111)
        ../src/subversion/libsvn_ra/ra_loader.c:500: (apr_err=111)
        svn: E000111: Unable to connect to a repository at URL
        'http://localhost:9630/obj/repo'
        ../src/subversion/libsvn_ra_serf/util.c:783: (apr_err=111)
        svn: E000111: Error running context: Connection refused
        
        Wireshark and the apache logs show no communication between socat and apache, so
        I think this is an incompatibility between serf and socat.
        

        Show
        philipm Philip Martin added a comment - socat appears to be affected as well. Running something like: socat -v TCP-LISTEN:9630,reuseaddr,fork TCP:localhost:8080 puts a socat proxy on port 9630 for apache on port 8080. This works when using neon but fails when using serf: ../src/subversion/svn/checkout-cmd.c:168: (apr_err=111) ../src/subversion/libsvn_client/checkout.c:137: (apr_err=111) ../src/subversion/libsvn_client/ra.c:476: (apr_err=111) ../src/subversion/libsvn_client/ra.c:336: (apr_err=111) ../src/subversion/libsvn_ra/ra_loader.c:500: (apr_err=111) svn: E000111: Unable to connect to a repository at URL 'http://localhost:9630/obj/repo' ../src/subversion/libsvn_ra_serf/util.c:783: (apr_err=111) svn: E000111: Error running context: Connection refused Wireshark and the apache logs show no communication between socat and apache, so I think this is an incompatibility between serf and socat.
        Hide
        philipm Philip Martin added a comment -

        The socat problem was an IPv6 versus IPv4 problem, the subversion client was
        trying IPv6 and failing. Changing the client to use 127.0.0.1 instead of
        localhost caused the client to use IPv4 ans socat works.
        
        That might mean that the squid error I reported earlier is not the chunked
        encoding error but simply a 4/6 mismatch.
        

        Show
        philipm Philip Martin added a comment - The socat problem was an IPv6 versus IPv4 problem, the subversion client was trying IPv6 and failing. Changing the client to use 127.0.0.1 instead of localhost caused the client to use IPv4 ans socat works. That might mean that the squid error I reported earlier is not the chunked encoding error but simply a 4/6 mismatch.
        Hide
        philipm Philip Martin added a comment -

        So the important error as far as squid is concerned is
        
        ../src/subversion/libsvn_ra_serf/util.c:773: (apr_err=175002)
        ../src/subversion/libsvn_ra_serf/util.c:2099: (apr_err=175002)
        ../src/subversion/libsvn_ra_serf/util.c:2099: (apr_err=175002)
        svn: E175002: OPTIONS request on '/obj/repo' failed: 501 Not Implemented
        
        which then gets wrapped up in the general unable to connect to repository.
        

        Show
        philipm Philip Martin added a comment - So the important error as far as squid is concerned is ../src/subversion/libsvn_ra_serf/util.c:773: (apr_err=175002) ../src/subversion/libsvn_ra_serf/util.c:2099: (apr_err=175002) ../src/subversion/libsvn_ra_serf/util.c:2099: (apr_err=175002) svn: E175002: OPTIONS request on '/obj/repo' failed: 501 Not Implemented which then gets wrapped up in the general unable to connect to repository.
        Hide
        gstein Greg Stein added a comment -

        The bulk of the work was committed in r1302682. We'll need a new API from serf, which will be released as 
        part of serf 1.1.x.
        
        

        Show
        gstein Greg Stein added a comment - The bulk of the work was committed in r1302682. We'll need a new API from serf, which will be released as part of serf 1.1.x.
        Hide
        cmpilato C. Michael Pilato added a comment -

        Serf 1.1.1 was just released.
        

        Show
        cmpilato C. Michael Pilato added a comment - Serf 1.1.1 was just released.
        Hide
        lgo Lieven Govaerts added a comment -

        Hm, there's nothing in 1.1.1 related to content-length.
        
        The diff of r1302682 points to new API serf_bucket_request_set_CL, which was released in serf 1.1.0 on 
        June 7, 2012.
        
        So this issue is solved.
        

        Show
        lgo Lieven Govaerts added a comment - Hm, there's nothing in 1.1.1 related to content-length. The diff of r1302682 points to new API serf_bucket_request_set_CL, which was released in serf 1.1.0 on June 7, 2012. So this issue is solved.
        Hide
        cmpilato C. Michael Pilato added a comment -

        Cool.  Thanks for bringing this to closure, Lieven.
        

        Show
        cmpilato C. Michael Pilato added a comment - Cool. Thanks for bringing this to closure, Lieven.
        Hide
        philipm Philip Martin added a comment -

        Support for serf is still not as good as neon. With neon I can use squid 3.1.6
        that comes with Debian stable. With serf using Subversion trunk@1395465 and serf
        1.1.x@1660 I still get:
        
        HTTP/1.0 501 Not Implemented
        Server: squid/3.1.6
        Mime-Version: 1.0
        Date: Mon, 08 Oct 2012 12:44:03 GMT
        Content-Type: text/html
        Content-Length: 3424
        X-Squid-Error: ERR_UNSUP_REQ 0
        Vary: Accept-Language
        Content-Language: en
        X-Cache: MISS from localhost
        X-Cache-Lookup: NONE from localhost:3128
        Via: 1.0 localhost (squid/3.1.6)
        Proxy-Connection: close
        
        with a body that includes:
        
        <div id="content">
        <p>The following error was encountered while trying to retrieve the URL: <a
        href="http://127.0.0.1:8888/obj/repo">http://127.0.0.1:8888/obj/repo</a></p>
        
        <blockquote id="error">
        <p><b>Unsupported Request Method and Protocol</b></p>
        </blockquote>
        
        
        

        Show
        philipm Philip Martin added a comment - Support for serf is still not as good as neon. With neon I can use squid 3.1.6 that comes with Debian stable. With serf using Subversion trunk@1395465 and serf 1.1.x@1660 I still get: HTTP/1.0 501 Not Implemented Server: squid/3.1.6 Mime-Version: 1.0 Date: Mon, 08 Oct 2012 12:44:03 GMT Content-Type: text/html Content-Length: 3424 X-Squid-Error: ERR_UNSUP_REQ 0 Vary: Accept-Language Content-Language: en X-Cache: MISS from localhost X-Cache-Lookup: NONE from localhost:3128 Via: 1.0 localhost (squid/3.1.6) Proxy-Connection: close with a body that includes: <div id="content"> <p>The following error was encountered while trying to retrieve the URL: <a href="http://127.0.0.1:8888/obj/repo">http://127.0.0.1:8888/obj/repo</a></p> <blockquote id="error"> <p><b>Unsupported Request Method and Protocol</b></p> </blockquote>
        Hide
        lgo Lieven Govaerts added a comment -

        The code wasn't activated yet.
        
        r1395777 should now fix this issue.
        
        I have chosen a different route than what I think were Greg's intentions:
        ra_serf will assume the server supports HTTP/1.1 which is our preferred HTTP version. So it will start 
        sending chunked pipelined requests. If there's a HTTP/1.0 proxy, it will return an error and HTTP/1.0 in 
        the status line. In case of squid that's a "411 Length required" error.
        
        Now I'm not sure that all proxies will return 411 to indicate fall back to HTTP/1.0.
        From serf issue 28 it seems that the perlbal proxy returns "400 Bad Request". r1395777 doesn't handle 
        that yet, but it's an issue fix which I'll make later if there are no objections to this approach.
        
        
        

        Show
        lgo Lieven Govaerts added a comment - The code wasn't activated yet. r1395777 should now fix this issue. I have chosen a different route than what I think were Greg's intentions: ra_serf will assume the server supports HTTP/1.1 which is our preferred HTTP version. So it will start sending chunked pipelined requests. If there's a HTTP/1.0 proxy, it will return an error and HTTP/1.0 in the status line. In case of squid that's a "411 Length required" error. Now I'm not sure that all proxies will return 411 to indicate fall back to HTTP/1.0. From serf issue 28 it seems that the perlbal proxy returns "400 Bad Request". r1395777 doesn't handle that yet, but it's an issue fix which I'll make later if there are no objections to this approach.
        Hide
        gstein Greg Stein added a comment -

        Unfortunately, that is backwards, and problematic. The intent (and proper operation) is to send an 
        HTTP/1.0-compatible request, detect the server understands 1.1, and switch to sending 1.1-compatible 
        messages.
        
        From RFC 2145, section 2:
        
        "One consequence of these rules is that an HTTP/1.1 message sent to an
           HTTP/1.0 recipient (or a recipient whose version is unknown) MUST be
           constructed so that it remains a valid HTTP/1.0 message when all
           headers not defined in the HTTP/1.0 specification [1] are removed."
        
        By sending the unknown (to a 1.0 server) Transfer-Coding: chunked header, you break this rule. The 
        request is not a valid 1.0 message.
        
        We know that the first request is an OPTIONS request, which is small and easy to serialize in order to get 
        a 1.0 Content-Length header.
        
        In libsvn_ra_serf/util.c:handle_response(), right after you parse the status-line, you can set the 'http10' 
        flag to FALSE if you find a more recent version of HTTP. All following requests will then use the chunked 
        encoding.
        
        

        Show
        gstein Greg Stein added a comment - Unfortunately, that is backwards, and problematic. The intent (and proper operation) is to send an HTTP/1.0-compatible request, detect the server understands 1.1, and switch to sending 1.1-compatible messages. From RFC 2145, section 2: "One consequence of these rules is that an HTTP/1.1 message sent to an HTTP/1.0 recipient (or a recipient whose version is unknown) MUST be constructed so that it remains a valid HTTP/1.0 message when all headers not defined in the HTTP/1.0 specification [1] are removed." By sending the unknown (to a 1.0 server) Transfer-Coding: chunked header, you break this rule. The request is not a valid 1.0 message. We know that the first request is an OPTIONS request, which is small and easy to serialize in order to get a 1.0 Content-Length header. In libsvn_ra_serf/util.c:handle_response(), right after you parse the status-line, you can set the 'http10' flag to FALSE if you find a more recent version of HTTP. All following requests will then use the chunked encoding.
        Hide
        lgo Lieven Govaerts added a comment -

        Some more info: I'm testing with squid 2.7.STABLE9, which returns 411.
        I overlooked your (Philip) previous message, but squid 3.1.6 returns 501 instead.
        
        In r1395786 I have added 400 and 501 as potential indications of a proxy not supporting HTTP/1.1.
        

        Show
        lgo Lieven Govaerts added a comment - Some more info: I'm testing with squid 2.7.STABLE9, which returns 411. I overlooked your (Philip) previous message, but squid 3.1.6 returns 501 instead. In r1395786 I have added 400 and 501 as potential indications of a proxy not supporting HTTP/1.1.
        Hide
        gstein Greg Stein added a comment -

        Oh. The code for resetting the http10 flag is already in there.
        

        Show
        gstein Greg Stein added a comment - Oh. The code for resetting the http10 flag is already in there.
        Hide
        gstein Greg Stein added a comment -

        (and you can see how brittle this approach is... the need to continue adding status codes which might 
        actually be trying to tell us something for real!)
        

        Show
        gstein Greg Stein added a comment - (and you can see how brittle this approach is... the need to continue adding status codes which might actually be trying to tell us something for real!)
        Hide
        lgo Lieven Govaerts added a comment -

        Hm, too bad, but if that's what the specs say then it has to be turned around.
        
        Simple fix, I'll do that tomorrow evening, too late for that now.
        
        

        Show
        lgo Lieven Govaerts added a comment - Hm, too bad, but if that's what the specs say then it has to be turned around. Simple fix, I'll do that tomorrow evening, too late for that now.
        Hide
        gstein Greg Stein added a comment -

        Coolio. Thanks, Lieven!
        

        Show
        gstein Greg Stein added a comment - Coolio. Thanks, Lieven!
        Hide
        lgo Lieven Govaerts added a comment -

        Greg: we can keep the Connection:keep:alive header for those HTTP/1.0 requests right? If proxies don't 
        support persistent connections (are there any?) they'll add the Connection:close header with every 
        response and serf will handle that. If they do, at least we can keep our pipelining model.
        

        Show
        lgo Lieven Govaerts added a comment - Greg: we can keep the Connection:keep:alive header for those HTTP/1.0 requests right? If proxies don't support persistent connections (are there any?) they'll add the Connection:close header with every response and serf will handle that. If they do, at least we can keep our pipelining model.
        Hide
        lgo Lieven Govaerts added a comment -

        r1396147 implements the mechanism as discussed: assume HTTP/1.0, upgrade to HTTP/1.1 if possible.
        
        Ready for test.
        

        Show
        lgo Lieven Govaerts added a comment - r1396147 implements the mechanism as discussed: assume HTTP/1.0, upgrade to HTTP/1.1 if possible. Ready for test.
        Hide
        gstein Greg Stein added a comment -

        Sure, the Keep-Alive can stick around. Some proxies may not handle it perfectly, but we can get closer to 
        correct operation, then start ironing out bad proxy behavior on that header.
        

        Show
        gstein Greg Stein added a comment - Sure, the Keep-Alive can stick around. Some proxies may not handle it perfectly, but we can get closer to correct operation, then start ironing out bad proxy behavior on that header.
        Hide
        lgo Lieven Govaerts added a comment -

        This issue was fixed since last reopened.
        

        Show
        lgo Lieven Govaerts added a comment - This issue was fixed since last reopened.

          People

          • Assignee:
            Unassigned
            Reporter:
            philipm Philip Martin
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development