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 -

          I'm not so sure this is correct.

          Jetty and other network servers worked perfectly well before the shutdown methods were added to the
          API. close also flushes data in buffers and does an orderly shutdown.

          Now a client may have an issue if it tries to write a request, close the output and read the response.
          But a) jetty is mainly a server; b) close is not called by the client or the server until the response is
          sent/received and complete; c) using close to signify the end of a request content is not legal HTTP.

          From what I can see, the implementations of the shutdown methods are very problematic.
          Many JVMs and socket types have them as not implemented.

          So unless you can demonstrate an actual loss of data due to an early close, then I'm not going
          to act on this issue. But I will continue to do a bit more research and any pointers you may have would
          be appreciated.

          Show
          Greg Wilkins added a comment - I'm not so sure this is correct. Jetty and other network servers worked perfectly well before the shutdown methods were added to the API. close also flushes data in buffers and does an orderly shutdown. Now a client may have an issue if it tries to write a request, close the output and read the response. But a) jetty is mainly a server; b) close is not called by the client or the server until the response is sent/received and complete; c) using close to signify the end of a request content is not legal HTTP. From what I can see, the implementations of the shutdown methods are very problematic. Many JVMs and socket types have them as not implemented. So unless you can demonstrate an actual loss of data due to an early close, then I'm not going to act on this issue. But I will continue to do a bit more research and any pointers you may have would be appreciated.
          Hide
          Kevin Conaway added a comment -

          From what I can see, the implementations of the shutdown methods are very problematic.
          Many JVMs and socket types have them as not implemented.

          Can you link to some research on that? I find that surprising.

          I'm attaching a test case that fails when sockets are being closed. It approximates what Restlet is doing for a chunked request.

          Show
          Kevin Conaway added a comment - From what I can see, the implementations of the shutdown methods are very problematic. Many JVMs and socket types have them as not implemented. Can you link to some research on that? I find that surprising. I'm attaching a test case that fails when sockets are being closed. It approximates what Restlet is doing for a chunked request.
          Hide
          Kevin Conaway added a comment -

          Test case that fails when SocketEndpoint.close() closes a client socket

          Show
          Kevin Conaway added a comment - Test case that fails when SocketEndpoint.close() closes a client socket
          Hide
          Kevin Conaway added a comment -

          Attaching a simplified version of the first test case.

          The problem seems to be related to

          • Chunked Encoding
          • Connection: close
          • Writing the "chunked" bytes one at a time
          Show
          Kevin Conaway added a comment - Attaching a simplified version of the first test case. The problem seems to be related to Chunked Encoding Connection: close Writing the "chunked" bytes one at a time
          Hide
          Kevin Conaway added a comment -

          The exception I get is:

          java.net.SocketException: Software caused connection abort: recv failed
          	at java.net.SocketInputStream.socketRead0(Native Method)
          	at java.net.SocketInputStream.read(SocketInputStream.java:129)
          	at java.net.SocketInputStream.read(SocketInputStream.java:182)
          	at org.mortbay.jetty.Jetty547TestCase2.read(Jetty547TestCase2.java:87)
          	at org.mortbay.jetty.Jetty547TestCase2.test547(Jetty547TestCase2.java:60)
          	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          	at java.lang.reflect.Method.invoke(Method.java:597)
          	at junit.framework.TestCase.runTest(TestCase.java:164)
          	at junit.framework.TestCase.runBare(TestCase.java:130)
          	at junit.framework.TestResult$1.protect(TestResult.java:106)
          	at junit.framework.TestResult.runProtected(TestResult.java:124)
          	at junit.framework.TestResult.run(TestResult.java:109)
          	at junit.framework.TestCase.run(TestCase.java:120)
          	at junit.framework.TestSuite.runTest(TestSuite.java:230)
          	at junit.framework.TestSuite.run(TestSuite.java:225)
          	at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130)
          	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
          	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:460)
          	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:673)
          	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:386)
          	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:196)
          
          Show
          Kevin Conaway added a comment - The exception I get is: java.net.SocketException: Software caused connection abort: recv failed at java.net.SocketInputStream.socketRead0(Native Method) at java.net.SocketInputStream.read(SocketInputStream.java:129) at java.net.SocketInputStream.read(SocketInputStream.java:182) at org.mortbay.jetty.Jetty547TestCase2.read(Jetty547TestCase2.java:87) at org.mortbay.jetty.Jetty547TestCase2.test547(Jetty547TestCase2.java:60) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at junit.framework.TestCase.runTest(TestCase.java:164) at junit.framework.TestCase.runBare(TestCase.java:130) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:120) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:460) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:673) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:386) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:196)
          Hide
          Greg Wilkins added a comment -

          Kevin,

          OK - you have me interested now!

          However, I note that the client does in fact received all the data.
          It is only on the read AFTER the last byte that the exception is thrown, when I thought -1 would
          have been a more correct response.

          So maybe the client needs to ignore reset responses if the connection:close header is set.

          It is rather strange, as wireshark shows that most closes are done with an orderly TCP/IP FIN and ACK
          only a few are actually terminated with a RST? shutdownOutput was added in java 1.3, so java 1.0 to 1.2 must have been able to do an orderly shutdown of sockets and I'm amazed they would break existing code by changing close().

          Anyway, I will play with shutdown output in jetty 7 and see if it causes any problems

          Show
          Greg Wilkins added a comment - Kevin, OK - you have me interested now! However, I note that the client does in fact received all the data. It is only on the read AFTER the last byte that the exception is thrown, when I thought -1 would have been a more correct response. So maybe the client needs to ignore reset responses if the connection:close header is set. It is rather strange, as wireshark shows that most closes are done with an orderly TCP/IP FIN and ACK only a few are actually terminated with a RST? shutdownOutput was added in java 1.3, so java 1.0 to 1.2 must have been able to do an orderly shutdown of sockets and I'm amazed they would break existing code by changing close(). Anyway, I will play with shutdown output in jetty 7 and see if it causes any problems
          Hide
          Greg Wilkins added a comment -

          The SelectChannelEndpoint already called shutdownOutput - just before it called close!
          I've modified SocketEndpoint to do the same, but for now I have left the close in.

          This appears to prevent the reset being sent!

          Can you test this solution. (note I think the client still needs to better handle RST).

          cheers

          Show
          Greg Wilkins added a comment - The SelectChannelEndpoint already called shutdownOutput - just before it called close! I've modified SocketEndpoint to do the same, but for now I have left the close in. This appears to prevent the reset being sent! Can you test this solution. (note I think the client still needs to better handle RST). cheers
          Hide
          Kevin Conaway added a comment -

          Jetty547TestCase3 still fails on the trunk. SocketEndPoint.close() still calls socket.close() and ChannelEndPoint.close() still calls channel.close()

          Show
          Kevin Conaway added a comment - Jetty547TestCase3 still fails on the trunk. SocketEndPoint.close() still calls socket.close() and ChannelEndPoint.close() still calls channel.close()
          Hide
          Greg Wilkins added a comment -

          Kevin,

          We have to call close, because shutdownOutputStream is not universally supported on all JVMs.
          On some socket types/JVMs it is implemented as a noop and on others it throws an unsupportedOperationException.

          I've discussed the issue with the IO guys at Sun, and they believe that close should always initiate an orderly shutdown. The fact that it is sometimes sending a RST appears to be a JVM bug. We can try calling shutdownOutput to attempt to avoid this JVM bug, but we can't avoid calling close to ensure that we really do close the connection.

          I suggest that you open a bug against the JVM for the RST sent on close. It simply should not do that.

          I'll have a look again at your test case, because when I last ran it, the shutdownOutput before the close made that test pass for me.

          Show
          Greg Wilkins added a comment - Kevin, We have to call close, because shutdownOutputStream is not universally supported on all JVMs. On some socket types/JVMs it is implemented as a noop and on others it throws an unsupportedOperationException. I've discussed the issue with the IO guys at Sun, and they believe that close should always initiate an orderly shutdown. The fact that it is sometimes sending a RST appears to be a JVM bug. We can try calling shutdownOutput to attempt to avoid this JVM bug, but we can't avoid calling close to ensure that we really do close the connection. I suggest that you open a bug against the JVM for the RST sent on close. It simply should not do that. I'll have a look again at your test case, because when I last ran it, the shutdownOutput before the close made that test pass for me.
          Hide
          Kevin Conaway added a comment -

          Can you provide more details on your discussions with the IO team at Sun? Is the problem that a RST gets sent for no reason and closes the socket?

          I'd like to have as much information as possible to file a bug report.

          Show
          Kevin Conaway added a comment - Can you provide more details on your discussions with the IO team at Sun? Is the problem that a RST gets sent for no reason and closes the socket? I'd like to have as much information as possible to file a bug report.
          Hide
          Greg Wilkins added a comment -

          It was an exchange while we were discussing the NIO2 API. I asked if a call to close should initiate a normal shutdown of the connection or not.
          The answer was that they believed that it should. When I said that sometimes we are observing RST, the response was simply along the lines that they didn't think that it should do that.

          I'm currently in transit, but will run your test harness again towards the end of the week.

          Show
          Greg Wilkins added a comment - It was an exchange while we were discussing the NIO2 API. I asked if a call to close should initiate a normal shutdown of the connection or not. The answer was that they believed that it should. When I said that sometimes we are observing RST, the response was simply along the lines that they didn't think that it should do that. I'm currently in transit, but will run your test harness again towards the end of the week.
          Hide
          Ville Sulko added a comment -

          I think I hit this exact problem when using apache as reverse proxy in front of jetty. As java documents [*] state, socket.close() means that the application is done with the socket, both input and output. As on tcp-level, the only way for system to close both input and output streams of a tcp connection, is to send a RST packet. And this is what java implementations do.They first send FIN/ACK for the output stream, followed by RST to close the input stream as well.

          This is not what apache mod_proxy expects. We have a case where client does a big http POST, say 2 megs. Apache proxy begins to transmit this request to the jetty. After reading a few kBs, jetty notices that the request needs to be authenticated. At this point jetty responds with HTTP 401, and closes the connection (socket.close() --> FIN + RST). Apache mod_proxy receives response and the FIN, immediately followed by RST-message.

          Now, what should mod_proxy do? TCP specification says that after RST all communication should immediately stop. Mod_proxy still has lots of request data to send. Some data hes been received, but so has RST. At this point, mod_proxy decides that the connection is broken, and result code HTTP 502 is returned to the client.

          I think this is a little gray area regarding the functionality of the HTTP protocol. Can server break the connection before the whole request is sent, and assume the client will handle the already-sent response? And of course it is pretty silly to send a 2 meg request twice, just to authenticate the user. All I know is, that currently this behaviour breaks proxied connections. If I could decide, I had this fixed in mod_proxy, as it would not break existing applications. However, jetty could provide an alternative (cleaner?) way of handling http-requests (consume all the request data before closing connection).

          [*] http://java.sun.com/j2se/1.5.0/docs/guide/net/articles/connection_release.html

          Show
          Ville Sulko added a comment - I think I hit this exact problem when using apache as reverse proxy in front of jetty. As java documents [*] state, socket.close() means that the application is done with the socket, both input and output. As on tcp-level, the only way for system to close both input and output streams of a tcp connection, is to send a RST packet. And this is what java implementations do.They first send FIN/ACK for the output stream, followed by RST to close the input stream as well. This is not what apache mod_proxy expects. We have a case where client does a big http POST, say 2 megs. Apache proxy begins to transmit this request to the jetty. After reading a few kBs, jetty notices that the request needs to be authenticated. At this point jetty responds with HTTP 401, and closes the connection (socket.close() --> FIN + RST). Apache mod_proxy receives response and the FIN, immediately followed by RST-message. Now, what should mod_proxy do? TCP specification says that after RST all communication should immediately stop. Mod_proxy still has lots of request data to send. Some data hes been received, but so has RST. At this point, mod_proxy decides that the connection is broken, and result code HTTP 502 is returned to the client. I think this is a little gray area regarding the functionality of the HTTP protocol. Can server break the connection before the whole request is sent, and assume the client will handle the already-sent response? And of course it is pretty silly to send a 2 meg request twice, just to authenticate the user. All I know is, that currently this behaviour breaks proxied connections. If I could decide, I had this fixed in mod_proxy, as it would not break existing applications. However, jetty could provide an alternative (cleaner?) way of handling http-requests (consume all the request data before closing connection). [*] http://java.sun.com/j2se/1.5.0/docs/guide/net/articles/connection_release.html
          Hide
          Ville Sulko added a comment -

          The more I think about this, the more I'm convinced that jetty behaves incorrectly. HTTP server cannot assume that the client reads the server response before the whole request has been sent. For example, apache mod_proxy sends the whole request before even trying to read the response. As jetty sends RST (socket.close()) before the whole request is read, the response is never even read.

          Show
          Ville Sulko added a comment - The more I think about this, the more I'm convinced that jetty behaves incorrectly. HTTP server cannot assume that the client reads the server response before the whole request has been sent. For example, apache mod_proxy sends the whole request before even trying to read the response. As jetty sends RST (socket.close()) before the whole request is read, the response is never even read.
          Hide
          Greg Wilkins added a comment -

          I'll ponder this for a little while.... but in terms of sending a 2M request twice because of authentication, the client really should be using Expect:Continues to avoid sending the body until the server has had a chance to send a 401.

          Show
          Greg Wilkins added a comment - I'll ponder this for a little while.... but in terms of sending a 2M request twice because of authentication, the client really should be using Expect:Continues to avoid sending the body until the server has had a chance to send a 401.
          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: