Jetty
  1. Jetty
  2. JETTY-547

Jetty should rely on socket.shutdownOutput() to close sockets

    Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 6.1.9
    • Fix Version/s: 6.1.26, 7.2.0
    • Component/s: HTTP, NIO
    • Labels:
      None
    • Patch Submitted:
      Yes
    • Number of attachments :
      4

      Description

      In both ChannelEndPoint and SocketEndPoint, jetty is calling close() on the associated client socket.

      This can lead to a race condition on the client side where jetty reads the request, writes the response and closes the socket before the client
      can actually read the socket.

      Jetty should only call socket.shutdownOutput() instead of close. shutdownOutput has the same effect of signaling that the socket is ready to be terminated but doesn't actually terminate it until the client does. From the javadoc:

      For a TCP socket, any previously written data will be sent followed by TCP's normal connection termination sequence

      This only happens when the Connection: close header is sent.

      The attached patch removes the socket.close() call from SocketEndPoint and removes the socket.close()/channel.close() calls from ChannelEndPoint

      1. jetty.patch
        2 kB
        Kevin Conaway
      2. Jetty547TestCase.java
        9 kB
        Kevin Conaway
      3. Jetty547TestCase2.java
        4 kB
        Kevin Conaway
      4. Jetty547TestCase3.java
        4 kB
        Greg Wilkins

        Issue Links

          Activity

          Hide
          Greg Wilkins added a comment -

          Ville,
          Once a non persistent response has been written, we are currently calling shutdownOutput and then close.
          It looks like it is pretty easy to delay the call to close until the request has been entirely read. I'm working on a patch for this and if it works it would be great if you could test this.

          I will probably only do this in jetty-7, as jetty-6 is in maint mode. Is this a problem for you?

          cheers

          Show
          Greg Wilkins added a comment - Ville, Once a non persistent response has been written, we are currently calling shutdownOutput and then close. It looks like it is pretty easy to delay the call to close until the request has been entirely read. I'm working on a patch for this and if it works it would be great if you could test this. I will probably only do this in jetty-7, as jetty-6 is in maint mode. Is this a problem for you? cheers
          Hide
          Greg Wilkins added a comment -

          I've committed a fix to jetty-7 r1948
          It would be really great if you could test ASAP as we have a release due.

          Show
          Greg Wilkins added a comment - I've committed a fix to jetty-7 r1948 It would be really great if you could test ASAP as we have a release due.
          Hide
          Ville Sulko added a comment -

          Thanks for the quick reply! I agree, that the client should use Expect/Continues. The client is Apache Maven 2.2.1. Unfortunately I have no easy way to see the headers of these requests, as they are using https. They are HTTP1.1 requests. There seems to be at least some discussion about the problem regarding maven, but I can't confirm if maven can support use of continues.

          In the middle, we have apache http, from RedHat EL5.5, version 2.2.3-43.el5. I think mod_proxy implements http1.1 compliant proxy, so it should support passing continues to the backend server. In our setup, httpd does ssl offloading, and proxies (reverse proxy) requests to the backend server. Many forums suggested forcing the proxy to use http1.0 for proxied requests as a solution to 50x errors, as well as disabling keepalives. Dumping backend server traffic however revealed this RST-behaviour, which causes 502 errors.

          On the backend server side we have Nexus (nexus.sonatype.org), which comes bundled with jetty 6.1.12. As this is a bundled setup, and I don't have access to this server, it may prove difficult to try this out. At least when it comes to upgrading major numbers...

          Show
          Ville Sulko added a comment - Thanks for the quick reply! I agree, that the client should use Expect/Continues. The client is Apache Maven 2.2.1. Unfortunately I have no easy way to see the headers of these requests, as they are using https. They are HTTP1.1 requests. There seems to be at least some discussion about the problem regarding maven, but I can't confirm if maven can support use of continues. In the middle, we have apache http, from RedHat EL5.5, version 2.2.3-43.el5. I think mod_proxy implements http1.1 compliant proxy, so it should support passing continues to the backend server. In our setup, httpd does ssl offloading, and proxies (reverse proxy) requests to the backend server. Many forums suggested forcing the proxy to use http1.0 for proxied requests as a solution to 50x errors, as well as disabling keepalives. Dumping backend server traffic however revealed this RST-behaviour, which causes 502 errors. On the backend server side we have Nexus (nexus.sonatype.org), which comes bundled with jetty 6.1.12. As this is a bundled setup, and I don't have access to this server, it may prove difficult to try this out. At least when it comes to upgrading major numbers...
          Hide
          Greg Wilkins added a comment -

          Well I'm not going to be able to fix a bundled 6.1.12 either.
          But if you raise an issue with the sonatype folks and point them to this issue, then on there next release cycle, we might be able to get something into a jetty-6 maintenance release for them.

          Show
          Greg Wilkins added a comment - Well I'm not going to be able to fix a bundled 6.1.12 either. But if you raise an issue with the sonatype folks and point them to this issue, then on there next release cycle, we might be able to get something into a jetty-6 maintenance release for them.
          Hide
          Greg Wilkins added a comment -

          Fixed for jetty-6 now also. r6192.

          Show
          Greg Wilkins added a comment - Fixed for jetty-6 now also. r6192.

            People

            • Assignee:
              Greg Wilkins
              Reporter:
              Kevin Conaway
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: