Call completeStream only once on failure. #13814

gregw · 2025-10-21T00:07:11Z

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback.

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

gregw · 2025-10-21T01:45:16Z

The issue is fundamentally that completeStream is being called twice.

For this to happen, we need a failure to be detected in the ChannelCallback.succeeded() method so that the following code isrun:

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

Lines 1573 to 1580 in 8dbd986

    
           if (failure != null) 
        
           { 
        
               httpChannelState._callbackFailure = failure; 
        
               if (!stream.isCommitted()) 
        
                   errorResponse = new ErrorResponse(request); 
        
               else 
        
                   completeStream = true; 
        
           }

This means that completeStream will be called, even though the other "legs of the 3 legged stool" are not complete - specifically we may still be inside the call to HandlerInvoker.run(), as in this stack for thread 258 :

2025-10-20 08:14:42,457 INFO  [WebServerImpl-258] trace.jetty.session.complete: complete() called on session [ManagedSession@4df349fb{id=MYSECRETSESSIONID,x=MYSECRETSESSIONID.node0,req=6,res=true}]
java.lang.Exception: complete stack
	...
	at org.eclipse.jetty.session.AbstractSessionManager.complete(AbstractSessionManager.java)
	at org.eclipse.jetty.session.AbstractSessionManager$SessionStreamWrapper.doComplete(AbstractSessionManager.java:1509)
	at org.eclipse.jetty.server.handler.ContextHandler$ScopedContext.run(ContextHandler.java:1518)
	at org.eclipse.jetty.session.AbstractSessionManager$SessionStreamWrapper.failed(AbstractSessionManager.java:1479)
	at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.completeStream(HttpChannelState.java:788)
	at org.eclipse.jetty.server.internal.HttpChannelState$ChannelCallback.succeeded(HttpChannelState.java:1591)
	at org.eclipse.jetty.server.handler.gzip.GzipResponseAndCallback.succeeded(GzipResponseAndCallback.java:95)
	at org.eclipse.jetty.ee10.servlet.ServletChannel.onCompleted(ServletChannel.java:765)
	at org.eclipse.jetty.ee10.servlet.ServletChannel.handle(ServletChannel.java:429)
	at org.eclipse.jetty.ee10.servlet.ServletHandler.handle(ServletHandler.java:470)
	at org.eclipse.jetty.ee10.servlet.SessionHandler.handle(SessionHandler.java:717)
	at org.eclipse.jetty.server.handler.ContextHandler.handle(ContextHandler.java:1071)
	at org.eclipse.jetty.server.Handler$Wrapper.handle(Handler.java:740)
	at org.eclipse.jetty.rewrite.handler.RewriteHandler.handle(RewriteHandler.java:138)
	at org.eclipse.jetty.server.handler.gzip.GzipHandler.handle(GzipHandler.java:611)
	at org.eclipse.jetty.server.Handler$Sequence.handle(Handler.java:805)
	at org.eclipse.jetty.server.Server.handle(Server.java:182)
	at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.run(HttpChannelState.java:677)
	at org.eclipse.jetty.util.thread.Invocable$ReadyTask.run(Invocable.java:177)
	at org.eclipse.jetty.http2.server.internal.HttpStreamOverHTTP2$1.run(HttpStreamOverHTTP2.java:144)
	...

We can see that there is a failure detected in ChannelCallback.succeeded() because SessionStreamWrapper.failed is ultimately called. This means that there must have been one of the following application errors:

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

Line 1543 in 8dbd986

failure = ExceptionUtil.combine(failure, new IllegalStateException("demand pending"));
jetty.project/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

Line 1545 in 8dbd986

failure = ExceptionUtil.combine(failure, new IllegalStateException("write pending"));
jetty.project/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

Line 1563 in 8dbd986

failure = ExceptionUtil.combine(failure, new IOException("content-length %d != %d written".formatted(committedContentLength, totalWritten)));

These are all plausible application errors, especially with something like server sent events.

So once thread 258 has called completeStream it returns all the way out of handling and it can be seen calling completeStream again, in this stack trace:

java.lang.Exception: complete stack
	...
	at org.eclipse.jetty.session.AbstractSessionManager.complete(AbstractSessionManager.java)
	at org.eclipse.jetty.session.AbstractSessionManager$SessionStreamWrapper.doComplete(AbstractSessionManager.java:1509)
	at org.eclipse.jetty.server.handler.ContextHandler$ScopedContext.run(ContextHandler.java:1524)
	at org.eclipse.jetty.session.AbstractSessionManager$SessionStreamWrapper.failed(AbstractSessionManager.java:1479)
	at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.completeStream(HttpChannelState.java:788)
	at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.run(HttpChannelState.java:712)
	at org.eclipse.jetty.util.thread.Invocable$ReadyTask.run(Invocable.java:177)
	at org.eclipse.jetty.http2.server.internal.HttpStreamOverHTTP2$1.run(HttpStreamOverHTTP2.java:144)
	...

which is called from this code after the handler has been invoked:

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

Lines 691 to 713 in 8dbd986

    
           try (AutoLock ignored = _lock.lock()) 
        
           { 
        
               stream = _stream; 
        
               _handling = null; 
        
               _handled = true; 
        
               failure = _callbackFailure; 
        
               callbackCompleted = _callbackCompleted; 
        
               lastStreamSendComplete = lockedIsLastStreamSendCompleted(); 
        
               completeStream = callbackCompleted && lastStreamSendComplete; 
        
               if (LOG.isDebugEnabled()) 
        
                   LOG.debug("handler invoked: completeStream={} failure={} callbackCompleted={} {}", completeStream, failure, callbackCompleted, HttpChannelState.this); 
        
           } 
        
           if (LOG.isDebugEnabled()) 
        
               LOG.debug("stream={}, failure={}, callbackCompleted={}, completeStream={}", stream, failure, callbackCompleted, completeStream); 
        
           if (completeStream) 
        
           { 
        
               if (LOG.isDebugEnabled()) 
        
                   LOG.debug("completeStream({}, {})", stream, Objects.toString(failure)); 
        
               completeStream(stream, failure); 
        
           }

Note that in order for this code to actually call completeStream then it must be true that completeStream = callbackCompleted && lastStreamSendComplete. Note that this is normally not the case for HTTP/1 because the first call to completeStream would have recycled the HttpChannelState and the callbackCompleted and lastStreamSendComplete will both be false. However, for H2, HttpChannelStates are re-used after being recycled, so another request may have come in and set the fields of the state again, so that the second call to completeStream incorrectly completes that new request.

Thus I believe the core fix is to not call completeStream whilst we are still handling. Furthermore, if we are to ignore the last write leg of the stool, we should explicitly force lastStreamSendComplete to true;

Unfortunately I have been unable to produce a unit test for this, as I believe it needs precisely unlucky timing and an application error.

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback.

gregw · 2025-10-21T01:54:55Z

@sbordet @lorban Can you review the diagnosis that @janbartel and I have come up with. I'm 90% sure this is it, but I cannot reproduce (any thoughts how we might be able to do that?).

gregw · 2025-10-21T02:00:48Z

Note that we added this completeStream call in #9684

sbordet

ChannelCallback.failed() seems not entirely correct either.

Can we write test cases for this scenario?

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

sbordet · 2025-10-21T20:24:39Z

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

+                        // We are committed and still handling, so let the HandlerInvoker complete, ignoring any pending reads/writes.
+                        httpChannelState._streamSendState = StreamSendState.LAST_COMPLETE;


Again, not sure we should ignore pending writes.

Or pending reads.... it is a difficult one. I'll at least change the code to enumerate the possible states

sbordet · 2025-10-21T20:24:42Z

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

-                    else
+                    else if (httpChannelState._handling == null)
+                        // We are committed, but no longer handling, so will complete here, ignoring any pending reads/writes
                        completeStream = true;


I don't think this is right.

If there is a write pending, we cannot complete the stream, or we bypass one leg.
I think we should call lockedIsLastStreamSendCompleted() here to figure out whether that leg is done.

gregw · 2025-10-21T20:44:31Z

ChannelCallback.failed() seems not entirely correct either.

@sbordet I will look....

Can we write test cases for this scenario?

Very hard, because unless there is another thread racing the second completeStream is not called. I'm open to suggestions.

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method.

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

gregw · 2025-10-22T09:56:56Z

@sbordet @lorban I'm getting concerned at the number of tests this PR is breaking in its current state.
I think we need to take time to consider the more "cleanup" changes and probably only make this in 12.1.x

So I propose that this PR should simply be 83c1718 for 12.0.x (perhaps with the EventSourceServlet cleanups), and then we can do a wider cleanup and refactor in 12.1.x next month.

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

lorban · 2025-10-23T12:31:22Z

Regarding the minimal 12.0 fix, I think httpChannelState._handling == null should actually be httpChannelState._handled

gregw · 2025-10-24T01:54:46Z

@lorban this is passing tests now.... so let's go for this one?
@sbordet @lorban Review please!!

sbordet · 2025-10-24T09:46:49Z

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

+                    Throwable unconsumed = stream.consumeAvailable();
+                    if (failure != null)
+                        ExceptionUtil.addSuppressedIfNotAssociated(failure, unconsumed);


Just ExceptionUtil.combine(failure, stream.consumeAvailable()); as the line above?
But then, this else block is identical to the else-if above?

sbordet · 2025-10-24T09:57:36Z

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/ExceptionUtil.java

+     * @param throwable The type of {@link Throwable} class to check association against. Can be null.
+     * @return true if the {@link Throwable} is associated with the provided type; otherwise, false.
+     */
+    public static boolean isAssociated(Throwable failure, Class<? extends Throwable> throwable)


Reads better hasAssociated() to me.

sbordet · 2025-10-24T10:00:55Z

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

+                        else if (response.lockedIsWriting())
+                        {
+                            // We are currently writing, so let the completion of that write handle the failure
+                            httpChannelState._callbackFailure = failure;


Unnecessary line.

sbordet · 2025-10-24T10:07:48Z

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

+                        else
+                        {
+                            // There has been no last write, but we will just fail the stream instead.
+                            completeStream = true;


Feels like it should be this, so the third leg is complete:

Suggested change

completeStream = true;

httpChannelState._streamSendState = StreamSendState.FAILED;

completeStream = true;

sbordet · 2025-10-24T10:17:59Z

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

+                    else if (!httpChannelState.lockedIsLastStreamSendCompleted() && !response.lockedIsWriting())
+                    {
+                        // last write is not going to happen after failure, so we can just fail anyway
+                        httpChannelState._streamSendState = StreamSendState.LAST_COMPLETE;
+                    }


If lockedIsWriting() I should do the same logic as above: steal the write callback, etc.

I would prefer this else-if to be written as just else, because the lack of a catch-all else branch feels like we're forgetting something: I prefer to have an explicit branch that says "here we should really do nothing".

sbordet · 2025-10-24T12:20:16Z

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

-    private class HandlerInvoker implements Invocable.Task, Callback
+    // HandlerInvoker is used as the Response's _writeCallback when ChannelCallback is succeeded and the last send still
+    // needs to be done, i.e.: _streamSendState set to LAST_SENDING by lockedLastStreamSend().
+    private class HandlerInvoker implements Task, Callback


I think we should split the functionality of this class.
Leave run() in HandlerInvoker, but move the Callback functionality into a LastStreamSendCallback class.

sbordet · 2025-10-24T12:42:45Z

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

+                            failedCallback = response._writeCallback;
+                            response._writeCallback = httpChannelState._handlerInvoker;


In this way, we may wait forever for the write to complete and invoke the _handlerInvoker.

How about this:

Suggested change

failedCallback = response._writeCallback;

response._writeCallback = httpChannelState._handlerInvoker;

Runnable task = response.lockedFailWrite(failure);

failedCallback = Callback.from(task, httpChannelState._handlerInvoker);

In 12.1.x we will leverage the new cancelSend() feature automatically.

sbordet · 2025-10-24T12:44:22Z

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

+        LAST_COMPLETE,
+
+        /** Failing, so last send will never happen */
+        FAILED


This state is only read, never assigned.

sbordet · 2025-10-24T13:00:10Z

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

            {
                assert _callbackCompleted;
                _streamSendState = StreamSendState.LAST_COMPLETE;
                completeStream = _handling == null;


Suggested change

completeStream = _handled;

sbordet · 2025-10-24T13:01:22Z

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


                if (writeFailure == NOTHING_TO_SEND)
                {
                    httpChannelState._writeInvoker.run(callback::succeeded);


Retain the InvocationType.

gregw closed this Oct 21, 2025

gregw force-pushed the fix/12.0.x/13470-completeStreamOnce branch from a8cf31d to bee65a6 Compare October 21, 2025 00:15

gregw mentioned this pull request Oct 21, 2025

Jetty 12.0: ManagedSession issues due to recursion and/pr multiple completions of the stream. #13470

Open

Call completeStream only once on failure.

daa9a47

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback.

gregw reopened this Oct 21, 2025

gregw linked an issue Oct 21, 2025 that may be closed by this pull request

Jetty 12.0: ManagedSession issues due to recursion and/pr multiple completions of the stream. #13470

Open

gregw commented Oct 21, 2025

View reviewed changes

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java Outdated Show resolved Hide resolved

Call completeStream only once on failure.

83c1718

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback.

gregw added the save-home label Oct 21, 2025

gregw added this to Jetty 12.0.30 Oct 21, 2025

gregw requested review from janbartel, lorban and sbordet October 21, 2025 20:08

sbordet requested changes Oct 21, 2025

View reviewed changes

gregw added 3 commits October 22, 2025 11:02

Call completeStream only once on failure.

2973561

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method.

Call completeStream only once on failure.

2f7aa04

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

Call completeStream only once on failure.

bcbf2c8

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

gregw requested a review from sbordet October 22, 2025 19:50

gregw added 3 commits October 23, 2025 07:33

Call completeStream only once on failure.

42e209f

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

Call completeStream only once on failure.

baca52e

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

Call completeStream only once on failure.

a326415

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

sbordet requested changes Oct 24, 2025

View reviewed changes

		// We are committed and still handling, so let the HandlerInvoker complete, ignoring any pending reads/writes.
		httpChannelState._streamSendState = StreamSendState.LAST_COMPLETE;

	completeStream = true;
	httpChannelState._streamSendState = StreamSendState.FAILED;
	completeStream = true;

		failedCallback = response._writeCallback;
		response._writeCallback = httpChannelState._handlerInvoker;

Call completeStream only once on failure. #13814

Are you sure you want to change the base?

Call completeStream only once on failure. #13814

Conversation

gregw commented Oct 21, 2025

Uh oh!

Uh oh!

gregw commented Oct 21, 2025

Uh oh!

gregw commented Oct 21, 2025

Uh oh!

gregw commented Oct 21, 2025

Uh oh!

sbordet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregw commented Oct 21, 2025

Uh oh!

gregw commented Oct 22, 2025

Uh oh!

lorban commented Oct 23, 2025

Uh oh!

gregw commented Oct 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants