-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Jetty 12 ee9/ee10 doesn't invoke callbacks when h2 client sends RST_STREAM #12313
Comments
Any triage/response for this, given the pending EOL on Jetty 11? We're trying to follow the Jetty team's advice and migrate away ASAP, but this seems like a pretty egregious bug. |
I'm looking into this. |
What happens on the server:
The last frame shows that we do not call The abort will in turn cause @niloc132 Jetty 11's behavior is wrong, as obviously the The correct behavior would be to notify Note also that your use case is racy by nature: if you have a failure arriving at exactly the same time you are writing, you may see |
Regarding writes, Jetty 12 EE10 behavior is the correct one. In Jetty 12 EE10, after 3 seconds the response is aborted and the In Jetty 12 EE9 the |
To confirm, I do consider it correct that if the client half closes (on a DATA or HEAD, I'm specifically not asking "is it correct that a write will cause an error", but "is it correct that a closed h2 stream will never result in notification on the server". That's the breakage here - I think I agree that Jetty 10/11 calling ReadListener.onError is technically wrong, but not getting any notification is far worse, and will prevent Jetty from being used as a gRPC server (along I'm sure with other use cases). If the write doesn't happen immediately, this bug transforms from "it is weird that I get this error now" to "we never get any notification that the client is gone".
I don't want to discount the need for handling a race, only to emphasize that this bug isn't about races, just about the failure of the server to notify the servlet and its listeners that the connection is dead (and in some cases, has been for many seconds, see below), without any possibility for a race.
I modified the example to not send payloads on a timer (emulating a case where the response won't be ready for some time, and the RST_STREAM is expected to be able to cancel no longer needed work). Jetty 12+EE9 will immediately call AsyncListener.onError, so that's good. I also increased the timeout from 10s to 100s to ensure that there was no chance of it interfering with the 3/4 second checks you mentioned, and then had the server write a response immediately, so that the server is effectively waiting on the client to send its next message, and thus must know if the client is still there. (EDIT: changes added in this commit: niloc132/jetty-stream-reset@0646f88) In Jetty 12 EE9, we wait a little over 100 seconds after the start of the call (that delay could account for the extra three seconds that the client delays before sending RST_STREAM? Again, this means that the server saw the RST_STREAM, but explicitly reset the timeout delay until that many seconds after the stream was terminated?
In Jetty 12 EE10, no errors occurred at all - there are no notifications at all to the server that the client is gone (since the server is waiting for the client's next message, which never comes because the stream is closed.
I manually shut down the server after 30 minutes of no notification. Neither of these outcomes are ideal - even if timeouts are set, we are required to send application-level messages to keep the stream alive, and the timeout has to be low enough to catch this lack of message, but high enough to avoid requiring sending no-op messages every second or whatnot. The fact that ee10 further never alerts on timeouts sounds like a separate bug... |
…ent sends RST_STREAM. * Fixed invocation of AsyncListener.onError(), now called even if the response is already committed, in both EE9 and EE10. * Reworked EE9 HttpChannel state machine in case of failures to be like EE10's. In particular, calling abort now is a state change, rather than a failure of the Handler callback. In this way, the handle() loop continues, enters case TERMINATED, and the callback is completed in onCompleted(). * Fixed EE9 handling of idle timeout in HttpChannel.onRequest(), that was missing. Signed-off-by: Simone Bordet <[email protected]>
…ent sends RST_STREAM. * Removed unnecessary calls to EE9's `HttpChannel.abort()`. Signed-off-by: Simone Bordet <[email protected]>
Thank you - I've built locally and can confirm that this issue appears to be resolved. I had a follow-up issue that we had experienced with Jetty 10/11 (AsyncContext.complete() seems to send RST_STREAM with error='cancel' instead of error='no error'), but that appears to also be resolved in Jetty 12. We'll continue our testing with the 12.0.15 release when available, but I'll run more grpc-java integration test locally to see if I can confirm that this snapshot passes other test too. Thanks again for the quick patch! |
@niloc132 just to be clear, I agree that upon receiving a RST_STREAM from the client, it must be notified to the server. There is actually a big hole in the specification about how exactly notify the application, see jakartaee/servlet#433. We'll try to do the right thing with the PR associated to this issue, so thanks for testing it. |
…ent sends RST_STREAM. * Do not recycle EE9 HttpChannel for WebSocket requests. * Simplified sendError() error handling. Signed-off-by: Simone Bordet <[email protected]>
Sorry for the misunderstanding on my part, I understand now that you were trying to acknowledge the issue but also point out that a race was possible (and must still be defended against). I'll follow that PR and re-test when merged, and when the release goes out. Aside, the RST_STREAM(cancel) issue is not fixed, I just tested it incorrectly before. We've confirmed that tomcat and undertow don't have this "bug", but it may well be a difference of interpretation in the spec - I'll try to write this up clearly as a separate Jetty issue, and go from there. Thanks again for your help with this bug. |
@niloc132 can you please elaborate on your testing results? I ask because now we have an explicit test that verifies that |
I'm sorry, that should have been more clear. I can confirm that with the linked PR, the example project behaves as I would expect in Jetty 12 EE9 and EE10 - AsyncListener.onError is called (where in Jetty 11 ReadListener.onError was previously called). I tested this with and without trying to write while waiting for the stream to be canceled by the client. Additionally, I can confirm that this patch works in our project, and resolves the issue there as well. I will file a separate issue for the other RST_STREAM confusion I am having, which may well be simply due to ambiguity in the spec. |
…ent sends RST_STREAM. * Fixed invocation of AsyncListener.onError(), now called even if the response is already committed, in both EE9 and EE10. * Reworked EE9 HttpChannel state machine in case of failures to be like EE10's. In particular, calling abort now is a state change, rather than a failure of the Handler callback. In this way, the handle() loop continues, enters case TERMINATED, and the callback is completed in onCompleted(). * Fixed EE9 handling of idle timeout in HttpChannel.onRequest(), that was missing. Signed-off-by: Simone Bordet <[email protected]>
…ent sends RST_STREAM. * Removed unnecessary calls to EE9's `HttpChannel.abort()`. Signed-off-by: Simone Bordet <[email protected]>
…ent sends RST_STREAM. * Do not recycle EE9 HttpChannel for WebSocket requests. * Simplified sendError() error handling. Signed-off-by: Simone Bordet <[email protected]>
I've tested this in our case and this eliminates a lot of warnings that we were getting from jersey when client aborted suspended requests in our code. Thanks! |
…ent sends RST_STREAM. * Fixed invocation of AsyncListener.onError(), now called even if the response is already committed, in both EE9 and EE10. * Reworked EE9 HttpChannel state machine in case of failures to be like EE10's. In particular, calling abort now is a state change, rather than a failure of the Handler callback. In this way, the handle() loop continues, enters case TERMINATED, and the callback is completed in onCompleted(). * Fixed EE9 handling of idle timeout in HttpChannel.onRequest(), that was missing. Signed-off-by: Simone Bordet <[email protected]>
Jetty version(s)
Compared Jetty 12.0.13 with Jetty 11.0.24
Jetty Environment
Tested EE9 and EE10
Java version/vendor
$ java -version openjdk version "21.0.4" 2024-07-16 OpenJDK Runtime Environment (build 21.0.4+7) OpenJDK 64-Bit Server VM (build 21.0.4+7, mixed mode, sharing)
OS type/version
$ uname -a Linux runes 6.10.4-arch2-1 #1 SMP PREEMPT_DYNAMIC Sun, 11 Aug 2024 16:19:06 +0000 x86_64 GNU/Linux
Description
When an h2 client disconnects from an async servlet by sending RST_STREAM, Jetty 11 (and earlier) would invoke onError callbacks. It seems that this no longer happens in Jetty 12 - you either have to try to write and see if it works, or wait for a timeout. The Jetty 12 behavior is slightly different for ee9 and ee10 - I believe both are wrong, but might be misinterpreting how a servlet implementation should handle this case?
I created an example async servlet that logs each event it receives, and compared behavior when a client connects, some content streamed back and forth, and the client disconnecting from the server with a RST_STREAM(CANCEL).
After that RST_STREAM, Jetty11 calls
ReadListener.onError
immediately. This seems reasonable to me, but I don't see an obvious reason in the spec that theAsyncListener.onError
couldn't be called instead.Under Jetty 12/EE9, instead there is no error when the RST_STREAM occurs, but instead
WriteListener.onError
is invoked on the first write by the server. This seems suspect, since it could be some time in a bidirectional stream before the server needs to send a message. Instead, the ReadListener probably should have its onError called, or possibly the AsyncListener.onError (and the caller probably then should avoid writing to begin with).Under Jetty 12/EE10, the error is a synchronously thrown exception - thrown by the ServletOutputStream.print/write calls. This seems to be different from servlet 3.1-9 implementation that I've worked with. Instead, as with Jetty 12/EE9, if the AsyncListener/ReadListener had one of their onError callbacks invoked, the writer could have known not to call to begin with.
For all three, the wire traffic is effectively the same, captured via wireshark - run in the same order as described above. This is to say that there doesn't seem to be any sort of bug outside of how Jetty notifies the running servlet that the error has occurred.
How to reproduce?
https://github.com/niloc132/jetty-stream-reset/tree/jetty-12313
The linked project contains some gradle setup to use the current latest releases of Jetty 11 and 12, and a simple Jetty 12 based client that will connect to any of them. The basis of the example is this servlet:
Each of the three server impls in the repo has this same body, with only changes made for the jakarta servlet package changes between releases/environments:
Note that this client uses h2c, but this behavior is observed in h2 (with tls) as well, which is how it was first observed.
The client in turn is simple, half closing after sending a simple payload:
The text was updated successfully, but these errors were encountered: