• Icon: Task Task
    • Resolution: Fixed
    • Icon: Major Major
    • core
    • None

      On the prototype branch of Jenkins core, Security3030Test fails with SocketException on Jetty 12. I have temporarily worked around the issue with the following hack:

      diff --git a/test/src/test/java/jenkins/security/Security3030Test.java b/test/src/test/java/jenkins/security/Security3030Test.java
      index e2d1e4bf5b..5a5ef07f85 100644
      --- a/test/src/test/java/jenkins/security/Security3030Test.java
      +++ b/test/src/test/java/jenkins/security/Security3030Test.java
      @@ -39,6 +39,7 @@ import java.io.OutputStream;
       import java.io.OutputStreamWriter;
       import java.io.PrintWriter;
       import java.lang.reflect.Field;
      +import java.net.SocketException;
       import java.net.URL;
       import java.nio.charset.StandardCharsets;
       import java.util.Random;
      @@ -212,7 +213,11 @@ public class Security3030Test {
               ByteArrayOutputStream baos = new ByteArrayOutputStream();
               writeMultipartFormDataBody(baos, boundary, files, other, fileSize);
               request.setRequestBody(baos.toString());
      -        wc.getPage(request);
      +        try {
      +            wc.getPage(request);
      +        } catch (SocketException e) {
      +            // TODO Why is this behavior changing?
      +        }
               return endpoint.getActual();
           }
      

      The root cause of this change in behavior should be understood. It may actually be a desirable Jetty security enhancement. If the behavior is expected, the test should be adapted (at the very least, to assert that this now expected exception actually occurs). If the behavior is unexpected, the bug should be fixed and the test change reverted. Before submitting the PR to the prototype branch to fix this ticket, ensure that mvn clean verify -Dtest=jenkins.security.Security3030Test passes on both Linux and Windows.

          [JENKINS-73127] Jetty 12 test failure in Security3030Test

          Basil Crow added a comment -

          I debugged this today and found that on Jetty 12 the server is correctly rejecting the request as before with the correct exception (no regression) but is now rejecting it so quickly that the server sends the rejection (or in the test's case, asserts the expected exception and then sends a success message, rather than sending the rejection as would be the case in production) before the client has finished sending the request. So the server sends the response and closes the socket, while the client still has the socket open and is still sending out the rest of the request. The client receives a broken pipe error because the server has closed the socket. This didn't happen in Jetty 10 presumably because the server was slower and the client would finish sending the request before the server had decided to reject it.

          I was able to get the test to pass in Jetty 12 easily enough:

          diff --git a/test/src/test/java/jenkins/security/Security3030Test.java b/test/src/test/java/jenkins/security/Security3030Test.java
          index 919faa5d7d..9a9f43ac52 100644
          --- a/test/src/test/java/jenkins/security/Security3030Test.java
          +++ b/test/src/test/java/jenkins/security/Security3030Test.java
          @@ -272,6 +272,10 @@ public class Security3030Test {
                           return processMultipartAndUnwrap(req);
                       } else {
                           actualWrapped = Assert.assertThrows(expectedWrapped, () -> processMultipartAndUnwrap(req));
          +                // The client might still be sending us more of the request, but we have had enough of it already and
          +                // have decided to stop processing it. Drain the read end of the socket so that the client can finish
          +                // sending its request in order to read the response we are about to provide.
          +                req.getInputStream().transferTo(OutputStream.nullOutputStream());
                           return HttpResponses.ok();
                       }
                   }
          

          The only remaining question in my mind is the following: is it OK that we're now rejecting these requests so quickly that clients might not even be done sending the request at the time that we reject the request and send the error back to the client? I personally think it is, and that the client should deal with this gracefully, something that HtmlUnit/Apache HttpClient 4.x (the client in the test mentioned above) isn't doing. The alternative, if we want to give this type of client the exact same behavior as before, would be to drain the read end of the socket in org.kohsuke.stapler.RequestImpl#parseMultipartFormData before throwing the ServletException, but to me this is a bad idea for a few reasons:

          • This could consume a lot of bandwidth if someone was maliciously sending enormous requests
          • It seems to be coddling poorly behaving clients unnecessarily

          So I think adapting the test as described above is the best solution. danielbeck do you have any opinion here?

          Basil Crow added a comment - I debugged this today and found that on Jetty 12 the server is correctly rejecting the request as before with the correct exception (no regression) but is now rejecting it so quickly that the server sends the rejection (or in the test's case, asserts the expected exception and then sends a success message, rather than sending the rejection as would be the case in production) before the client has finished sending the request. So the server sends the response and closes the socket, while the client still has the socket open and is still sending out the rest of the request. The client receives a broken pipe error because the server has closed the socket. This didn't happen in Jetty 10 presumably because the server was slower and the client would finish sending the request before the server had decided to reject it. I was able to get the test to pass in Jetty 12 easily enough: diff --git a/test/src/test/java/jenkins/security/Security3030Test.java b/test/src/test/java/jenkins/security/Security3030Test.java index 919faa5d7d..9a9f43ac52 100644 --- a/test/src/test/java/jenkins/security/Security3030Test.java +++ b/test/src/test/java/jenkins/security/Security3030Test.java @@ -272,6 +272,10 @@ public class Security3030Test { return processMultipartAndUnwrap(req); } else { actualWrapped = Assert.assertThrows(expectedWrapped, () -> processMultipartAndUnwrap(req)); + // The client might still be sending us more of the request, but we have had enough of it already and + // have decided to stop processing it. Drain the read end of the socket so that the client can finish + // sending its request in order to read the response we are about to provide. + req.getInputStream().transferTo(OutputStream.nullOutputStream()); return HttpResponses.ok(); } } The only remaining question in my mind is the following: is it OK that we're now rejecting these requests so quickly that clients might not even be done sending the request at the time that we reject the request and send the error back to the client? I personally think it is, and that the client should deal with this gracefully, something that HtmlUnit/Apache HttpClient 4.x (the client in the test mentioned above) isn't doing. The alternative, if we want to give this type of client the exact same behavior as before, would be to drain the read end of the socket in org.kohsuke.stapler.RequestImpl#parseMultipartFormData before throwing the ServletException , but to me this is a bad idea for a few reasons: This could consume a lot of bandwidth if someone was maliciously sending enormous requests It seems to be coddling poorly behaving clients unnecessarily So I think adapting the test as described above is the best solution. danielbeck do you have any opinion here?

          Daniel Beck added a comment -

          is it OK that we're now rejecting these requests so quickly that clients might not even be done sending the request at the time that we reject the request and send the error back to the client? I personally think it is

          I agree.

          Daniel Beck added a comment - is it OK that we're now rejecting these requests so quickly that clients might not even be done sending the request at the time that we reject the request and send the error back to the client? I personally think it is I agree.

          Basil Crow added a comment -

          The test is passing on both Linux and Windows with this workaround:

          diff --git a/test/src/test/java/jenkins/security/Security3030Test.java b/test/src/test/java/jenkins/security/Security3030Test.java
          index 919faa5d7d..5e712bdc4e 100644
          --- a/test/src/test/java/jenkins/security/Security3030Test.java
          +++ b/test/src/test/java/jenkins/security/Security3030Test.java
          @@ -272,6 +272,14 @@ public class Security3030Test {
                           return processMultipartAndUnwrap(req);
                       } else {
                           actualWrapped = Assert.assertThrows(expectedWrapped, () -> processMultipartAndUnwrap(req));
          +
          +                // The client might still be sending us more of the request, but we have had enough of it already and
          +                // have decided to stop processing it. Drain the read end of the socket so that the client can finish
          +                // sending its request in order to read the response we are about to provide.
          +                try (OutputStream os = OutputStream.nullOutputStream()) {
          +                    req.getInputStream().transferTo(os);
          +                }
          +
                           return HttpResponses.ok();
                       }
                   }
          

          I don't think it's worth trying to fix the HTTP client or to change the production behavior, so from my perspective this issue is done.

          Basil Crow added a comment - The test is passing on both Linux and Windows with this workaround: diff --git a/test/src/test/java/jenkins/security/Security3030Test.java b/test/src/test/java/jenkins/security/Security3030Test.java index 919faa5d7d..5e712bdc4e 100644 --- a/test/src/test/java/jenkins/security/Security3030Test.java +++ b/test/src/test/java/jenkins/security/Security3030Test.java @@ -272,6 +272,14 @@ public class Security3030Test { return processMultipartAndUnwrap(req); } else { actualWrapped = Assert.assertThrows(expectedWrapped, () -> processMultipartAndUnwrap(req)); + + // The client might still be sending us more of the request, but we have had enough of it already and + // have decided to stop processing it. Drain the read end of the socket so that the client can finish + // sending its request in order to read the response we are about to provide. + try (OutputStream os = OutputStream.nullOutputStream()) { + req.getInputStream().transferTo(os); + } + return HttpResponses.ok(); } } I don't think it's worth trying to fix the HTTP client or to change the production behavior, so from my perspective this issue is done.

            basil Basil Crow
            basil Basil Crow
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated:
              Resolved: