Fixes #11016 - Jetty 12 IllegalStateException when stopping Server wi… #11017

sbordet · 2023-12-03T17:47:31Z

…th pending requests

Made ServletChannel error handling more robust. A failure in error handling is now remembered so that the Handler callback can be failed later.
Avoid failing the Handler callback from ServletChannel.abort(), as it is too early: should be failed when processing the TERMINATED state, similarly to when it is succeeded.
Removed dead code from HttpConnection.SendCallback.reset(), since response is always non-null.

…th pending requests * Made ServletChannel error handling more robust. A failure in error handling is now remembered so that the Handler callback can be failed later. * Avoid failing the Handler callback from ServletChannel.abort(), as it is too early: should be failed when processing the TERMINATED state, similarly to when it is succeeded. * Removed dead code from HttpConnection.SendCallback.reset(), since response is always non-null. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

…ed(). This may run the ErrorHandler twice when using ee10. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw

Mostly just more comments/javadoc needed... but I think there may be a problem with

jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java

gregw · 2023-12-04T01:19:23Z

...e10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java

@@ -292,19 +293,13 @@ public String getStatusString()
        }
    }

-    public boolean completeResponse()
+    public Throwable completeResponse()


The boolean return was giving protection against calling this method twice.
So either, there is a possibility that we will call it twice, in which case we still need the protection....
OR we will never call it twice, so instead we should throw ISE if we are ever called with it is already in COMPLETED state.

This method is only called once because it is protected by the ServletChannel state machine.
From ServletChannel.handle(), we end up in the TERMINATED state where we call onCompleted() that calls this method.

So can we have an assert (or ISE) if it is already in COMPLETED state

Not sure what you refer to, as there are many COMPLETED states?
If you refer to _outputState it can also be ABORTED.
I feel like we don't need this extra assert, as we just want to make the transition OPEN -> COMPLETED.

This method used to return a boolean that indicated if it had just transitioned from OPEN->COMPLETED. If it returned false, then the callback would not be completed.

Now it returns a throwable to say if there has been a failure, in which case the callback is failed. But it has lost the protection for multiple calls, so the callback might be called twice.

I understand that you know this cannot happen because of external factors.... that nothing in this method represents that invariant. It would be clearer if it had something like:

assert _failure != null || _outputState == OutputState.OPEN

If this assert does not hold, then this PR has changed behaviour in a non obvious way.

...e10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java

…pended-request-ise'.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw

All good, but a couple of nigglets.

gregw · 2023-12-15T00:27:32Z

...e10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java

@@ -292,19 +293,13 @@ public String getStatusString()
        }
    }

-    public boolean completeResponse()
+    public Throwable completeResponse()


So can we have an assert (or ISE) if it is already in COMPLETED state

gregw · 2023-12-15T00:28:31Z

jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java

@@ -735,10 +729,13 @@ public void onCompleted()
            _servletContextRequest.setAttribute(CustomRequestLog.LOG_DETAIL, logDetail);
        }

-        // Callback will either be succeeded here or failed in abort().
+        // Callback is completed here.


Can we say:

Suggested change

// Callback is completed here.

// Callback is only completed here.

…pended-request-ise'. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw · 2023-12-15T09:54:48Z

...e10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java

@@ -292,19 +293,13 @@ public String getStatusString()
        }
    }

-    public boolean completeResponse()
+    public Throwable completeResponse()


This method used to return a boolean that indicated if it had just transitioned from OPEN->COMPLETED. If it returned false, then the callback would not be completed.

Now it returns a throwable to say if there has been a failure, in which case the callback is failed. But it has lost the protection for multiple calls, so the callback might be called twice.

I understand that you know this cannot happen because of external factors.... that nothing in this method represents that invariant. It would be clearer if it had something like:

assert _failure != null || _outputState == OutputState.OPEN

If this assert does not hold, then this PR has changed behaviour in a non obvious way.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested a review from gregw December 3, 2023 17:47

sbordet linked an issue Dec 3, 2023 that may be closed by this pull request

IllegalStateException when stopping Server with pending requests #11016

Closed

sbordet added the build-no-cache label Dec 3, 2023

sbordet added 3 commits December 3, 2023 19:16

Restored check of stream.isCommitted() rather than response.isCommitt…

6c839f6

…ed(). This may run the ErrorHandler twice when using ee10. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Fixed dependency to awaitility in jetty-ee8-servlet.

efee6f0

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Improved handling of abort().

d93cba7

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw requested changes Dec 4, 2023

View reviewed changes

joakime linked an issue Dec 13, 2023 that may be closed by this pull request

Flaky test MultiPartServletTest.testMultiPartGzip #10944

Closed

sbordet added 2 commits December 14, 2023 09:23

Merged branch 'jetty-12.0.x' into 'fix/jetty-12/11016/server-stop-sus…

ac47524

…pended-request-ise'.

Updates from review.

586d57a

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested a review from gregw December 14, 2023 09:13

gregw previously approved these changes Dec 15, 2023

View reviewed changes

Merged branch 'jetty-12-0.x' into 'fix/jetty-12/11016/server-stop-sus…

6377c57

…pended-request-ise'. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet dismissed gregw’s stale review via 6377c57 December 15, 2023 08:10

Updates from review.

ad46276

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw previously approved these changes Dec 15, 2023

View reviewed changes

Updates from review.

80c183c

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet dismissed gregw’s stale review via 80c183c December 15, 2023 14:07

sbordet merged commit eb1e9eb into jetty-12.0.x Dec 15, 2023
2 of 4 checks passed

sbordet deleted the fix/jetty-12/11016/server-stop-suspended-request-ise branch December 15, 2023 14:09

sbordet mentioned this pull request Dec 15, 2023

IllegalStateException when stopping Server with pending requests #11016

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #11016 - Jetty 12 IllegalStateException when stopping Server wi… #11017

Fixes #11016 - Jetty 12 IllegalStateException when stopping Server wi… #11017

sbordet commented Dec 3, 2023

gregw left a comment

gregw Dec 4, 2023

sbordet Dec 14, 2023

gregw Dec 15, 2023

sbordet Dec 15, 2023

gregw Dec 15, 2023

gregw left a comment

gregw Dec 15, 2023

gregw Dec 15, 2023

gregw Dec 15, 2023

	// Callback is completed here.
	// Callback is only completed here.

Fixes #11016 - Jetty 12 IllegalStateException when stopping Server wi… #11017

Fixes #11016 - Jetty 12 IllegalStateException when stopping Server wi… #11017

Conversation

sbordet commented Dec 3, 2023

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment