Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client hangs despite setting timeout #11894

Open
garydgregory opened this issue Jun 9, 2024 · 3 comments
Open

Client hangs despite setting timeout #11894

garydgregory opened this issue Jun 9, 2024 · 3 comments
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@garydgregory
Copy link
Contributor

garydgregory commented Jun 9, 2024

Jetty version(s)
12.0.10-SNAPSHOT

Jetty Environment
ee9

Java version/vendor (use: java -version)

openjdk version "17.0.11" 2024-04-16
OpenJDK Runtime Environment Temurin-17.0.11+9 (build 17.0.11+9)
OpenJDK 64-Bit Server VM Temurin-17.0.11+9 (build 17.0.11+9, mixed mode, sharing)

OS type/version
Microsoft Windows [Version 10.0.19045.4412]

Description
To even give the test below a chance to run, I must apply PR #11891 to avoid NPEs on the server side. For me, the test hangs after between 90 and 400 iterations. If it does not, you can stress your CPU to 92%+ with CPU Stres.

Based on some println()s here and there (logging can affect timings):
The hang is cause by CompletableFuture.get() never returning in listener.send().get() in HttpRequest.send():

    public ContentResponse send() throws InterruptedException, TimeoutException, ExecutionException
    {
        try
        {
            CompletableResponseListener listener = new CompletableResponseListener(this);
            return listener.send().get();
        }
    ...

get never returns because CompletableResponseListener.onComplete(Result) is never called,
because ResponseListeners.notifyComplete(Response.CompleteListener, Result) is never called,
because ResponseListeners.notifyComplete(Result) is never called.

How to reproduce?

diff --git a/jetty-ee9/jetty-ee9-proxy/src/test/java/org/eclipse/jetty/ee9/proxy/AsyncMiddleManServletTest.java b/jetty-ee9/jetty-ee9-proxy/src/test/java/org/eclipse/jetty/ee9/proxy/AsyncMiddleManServletTest.java
index 571b223307..1d171ffa17 100644
--- a/jetty-ee9/jetty-ee9-proxy/src/test/java/org/eclipse/jetty/ee9/proxy/AsyncMiddleManServletTest.java
+++ b/jetty-ee9/jetty-ee9-proxy/src/test/java/org/eclipse/jetty/ee9/proxy/AsyncMiddleManServletTest.java
@@ -41,6 +41,7 @@ import java.util.concurrent.ThreadLocalRandom;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.zip.GZIPInputStream;
 import java.util.zip.GZIPOutputStream;
 import java.util.zip.ZipEntry;
@@ -289,6 +290,72 @@ public class AsyncMiddleManServletTest
         }
     }

+    /**
+     * Tests that we do not hang processing sequenced client requests that are fast on the server-side;
+     * requires testServletOnContinueNullPointerException() to pass as a prerequisite.
+     * <p>
+     * Commenting out client EXPECT processing allows the test not to hang.
+     * </p>
+     * <p>
+     * It does not always happen, it may be related to how busy the CPU is; using "CPU Stres" from
+     * https://learn.microsoft.com/en-us/sysinternals/downloads/cpustres helps make the failure happen more often when
+     * I peg the CPU at 92% busy or above.
+     * </p>
+     * @throws Exception on a test failure.
+     */
+    @Test
+    public void testServletExpect100Hanging() throws Exception
+    {
+        startServer(new HttpServlet()
+        {
+            private AtomicInteger count = new AtomicInteger();
+
+            @Override
+            protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException
+            {
+                // Adding logging may slow things down enough to not hang
+                // System.out.printf("Origin servicing %d%n", count.incrementAndGet());
+                // Send the 100 Continue.
+                ServletInputStream input = request.getInputStream();
+                // Echo the content.
+                IO.copy(input, response.getOutputStream());
+                // Slowing down the origin allows the test to not hang sometimes
+//                try {
+//                    Thread.sleep(50);
+//                } catch (InterruptedException e) {
+//                    e.printStackTrace();
+//                }
+            }
+        });
+        startProxy(new AsyncMiddleManServlet() {
+            private AtomicInteger count = new AtomicInteger();
+
+            @Override
+            protected void service(HttpServletRequest clientRequest, HttpServletResponse proxyResponse) throws ServletException, IOException {
+                // Adding logging may slow things down enough to not hang
+                // System.out.printf("Middle servicing %d%n", count.incrementAndGet());
+                super.service(clientRequest, proxyResponse);
+            }
+        });
+        startClient();
+
+        // loop to attempt increase odds of hanging
+        for (int i = 1; i <= 500; i++)
+        {
+            long start = System.currentTimeMillis();
+            System.out.printf("Sending %d", i);
+            ContentResponse response = client.newRequest("localhost", serverConnector.getLocalPort())
+                    .timeout(5, TimeUnit.SECONDS)
+                    // Commenting out EXPECT processing allows the test not to hang.
+                    .headers(headers -> headers.put(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE))
+                    // Hangs with small payloads, 10k won't hang, so test smallest payload.
+                    .body(new BytesRequestContent("a"))
+                    .send();
+            assertEquals(200, response.getStatus());
+            System.out.printf(" %,d%n", System.currentTimeMillis() - start);
+        }
+    }
+
     @Test
     public void testClientRequestSmallContentKnownLengthGzipped() throws Exception
     {
@janbartel
Copy link
Contributor

@lorban any insight on this one?

@lorban
Copy link
Contributor

lorban commented Jul 12, 2024

We started investigating with @sbordet and it looks like there is a race condition in the proxy code. More work is needed to really understand what's going on and to come up with a fix.

Stay tuned.

@garydgregory
Copy link
Contributor Author

@lorban
Thank you for the update! Looking forward to a solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

No branches or pull requests

4 participants