Fixes #8211 - fix ISE from HttpChannelState if failure during process (…

…#8215)

Now all the logic for completion is in lockedOnComplete().

Avoid throwing in ChannelCallback.succeeded() if HttpChannelState._error != null.
This is necessary because HttpChannelState._error may be set asynchronously by
some event such as HTTP/2 reset streams or idle timeouts, but if there is a
thread dispatched to the application the asynchronous event will not fail the
callback, as the failure may be noticed by the application (e.g. via a read() call).

Fixed TrailersTest.testHandlerRequestTrailers() by avoid reading again after
having read the trailers.

Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
Co-authored-by: Simone Bordet <[email protected]>

jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/IdleTimeoutTest.java

@@ -49,6 +49,7 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
@@ -332,13 +333,34 @@ public void onHeaders(Stream stream, HeadersFrame frame)
     public void testClientEnforcingStreamIdleTimeout() throws Exception
     {
         int idleTimeout = 1000;
+        AtomicReference<Throwable> thrownByCallback = new AtomicReference<>();
         start(new Handler.Processor()
         {
             @Override
             public void process(Request request, Response response, Callback callback)
             {
                 sleep(2 * idleTimeout);
-                callback.succeeded();
+
+                try
+                {
+                    callback.succeeded();
+                }
+                catch (Throwable x)
+                {
+                    // Callback.succeeded() must not throw.
+                    thrownByCallback.set(x);
+                }
+
+                try
+                {
+                    // Wrongly calling callback.failed() after succeeded() must not throw.
+                    callback.failed(new Throwable("thrown by test"));
+                }
+                catch (Throwable x)
+                {
+                    // Callback.failed() must not throw.
+                    thrownByCallback.set(x);
+                }
             }
         });
 
@@ -376,7 +398,13 @@ public boolean onIdleTimeout(Stream stream, Throwable x)
 
         assertTrue(timeoutLatch.await(5, TimeUnit.SECONDS));
         // We must not receive any DATA frame.
+        // This is because while the server receives a reset, there is a thread
+        // dispatched to the application, which could (but in this test does not)
+        // read from the request to notice the reset, so the server processing
+        // completes successfully with 200 and no DATA, rather than a 500 with DATA.
         assertFalse(dataLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS));
+        // No exceptions thrown by the callback on server.
+        assertNull(thrownByCallback.get());
         // Stream must be gone.
         assertTrue(session.getStreams().isEmpty());
         // Session must not be closed, nor disconnected.

jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/TrailersTest.java

@@ -146,6 +146,7 @@ private void otherReads()
                         HttpFields trailers = contentTrailers.getTrailers();
                         assertNotNull(trailers.get("X-Trailer"));
                         _callback.succeeded();
+                        return;
                     }
                 }
             }

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

@@ -424,7 +424,7 @@ public Invocable.InvocationType getInvocationType()
     public Runnable onFailure(Throwable x)
     {
         if (LOG.isDebugEnabled())
-            LOG.debug("onError {}", this, x);
+            LOG.debug("onFailure {}", this, x);
 
         HttpStream stream;
         Runnable task;
@@ -663,8 +663,9 @@ public void run()
             {
                 failure = x;
             }
+
             if (failure != null)
-                request._callback.failed(failure);
+                _request._callback.failed(failure);
 
             HttpStream stream;
             boolean completeStream;
@@ -883,6 +884,7 @@ HttpChannelState getHttpChannel()
 
         private HttpChannelState lockedGetHttpChannel()
         {
+            assert _lock.isHeldByCurrentThread();
             if (_httpChannel == null)
                 throw new IllegalStateException("channel already completed");
             return _httpChannel;
@@ -1317,7 +1319,8 @@ public void succeeded()
             boolean completeStream;
             try (AutoLock ignored = _request._lock.lock())
             {
-                lockedOnComplete();
+                if (!lockedOnComplete())
+                    return;
                 httpChannelState = _request._httpChannel;
                 completeStream = httpChannelState._processState == ProcessState.PROCESSED && httpChannelState._writeState == WriteState.LAST_WRITE_COMPLETED;
 
@@ -1327,8 +1330,8 @@ public void succeeded()
                     throw new IllegalStateException("demand pending");
                 if (httpChannelState._writeCallback != null)
                     throw new IllegalStateException("write pending");
-                if (httpChannelState._error != null)
-                    throw new IllegalStateException("error " + httpChannelState._error, httpChannelState._error.getCause());
+                // Here, httpChannelState._error might have been set by some
+                // asynchronous event such as an idle timeout, and that's ok.
 
                 needLastWrite = switch (httpChannelState._writeState)
                 {
@@ -1377,8 +1380,8 @@ public void failed(Throwable failure)
             boolean completeStream;
             try (AutoLock ignored = _request._lock.lock())
             {
-                lockedOnComplete();
-
+                if (!lockedOnComplete())
+                    return;
                 httpChannelState = _request._httpChannel;
                 httpChannelState._failure = failure;
                 completeStream = httpChannelState._processState == ProcessState.PROCESSED;
@@ -1416,27 +1419,31 @@ else if (completeStream)
             }
         }
 
-        private void lockedOnComplete()
+        private boolean lockedOnComplete()
         {
             assert _request._lock.isHeldByCurrentThread();
 
             HttpChannelState httpChannelState = _request._httpChannel;
             if (httpChannelState == null)
             {
                 if (LOG.isDebugEnabled())
-                    LOG.warn("already recycled after completion {} by", _request, _completedBy);
-                throw new IllegalStateException("channel already completed");
+                    LOG.debug("already recycled after completion {} by", _request, _completedBy);
+                return false;
             }
 
             if (httpChannelState._completed)
             {
                 if (LOG.isDebugEnabled())
-                    LOG.warn("already completed {} by", _request, _completedBy);
-                throw new IllegalStateException("already completed");
+                    LOG.debug("already completed {} by", _request, _completedBy);
+                return false;
             }
+
             if (LOG.isDebugEnabled())
                 _completedBy = new Throwable(Thread.currentThread().getName());
+
             httpChannelState._completed = true;
+
+            return true;
         }
 
         @Override