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

Review ServletChannelState.asyncError() #10933

Closed
sbordet opened this issue Nov 28, 2023 · 2 comments · Fixed by #10949
Closed

Review ServletChannelState.asyncError() #10933

sbordet opened this issue Nov 28, 2023 · 2 comments · Fixed by #10949
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@sbordet
Copy link
Contributor

sbordet commented Nov 28, 2023

Jetty version(s)
12

Description
ServletChannelState.asyncError() is currently not called, but we could wire it again so that if a Servlet calls startAsync(), we register a failure listener that calls asyncError() so Servlet may be notified of asynchronous errors.

@gregw
Copy link
Contributor

gregw commented Dec 5, 2023

@sbordet @janbartel @lorban @lachlan-roberts

In core, we use the ErrorHandler when the Callback passed to handle(...) is completed unsuccessfully (either failed directly or succeed call in a bad state (e.g insufficient content written). To call the ErrorHandler, we:

  • wrap the response with an HttpChannelState.ErrorResponse, which ignores channel failures so a response can be written
  • creates a new HttpChannelState.ErrorCallback
  • calls Response#writeError(request, errorResponse, errorCallback, int, java.lang.String, java.lang.Throwable)
  • wrap the request with ErrorHandler.ErrorRequest, which adds error attributes and disables the read API
  • calls the ErrorHandler.handle(...) method

This allows core to write an error response even if there has been a failure.

However, in ee10 Servlets, we do not use this mechanism. Instead handling SEND_ERROR calls the ErrorHandler directly with the unwrapped response and a new Callback that calls ServletChannelState#errorHandlingComplete.
The problem here, is that because the response is not wrapped, any core failures will make the write fail. We could consider doing the same kind of wrapping as in core, but in ee10, we don't really have the ability to bypass and go directly to the HttpStream.

Moreover, that does not help with the use-case of a HttpChannel.onFailure being passed to an AsyncListener.onError method that wishes to write an error response, but can't be all writes fail because of the core HttpChannel failure, which is not ignored.

The problem with the async use-case, is that the async listener will use the original request/response object and thus cannot be given any wrapper. So if we do use an ErrorResponse approach, then it needs to be inserted underneath the ServletApiResponse to be used by the servlet API once we are in error handling mode.

@gregw gregw moved this to 🏗 In progress in Jetty 12.0.5 - FROZEN Dec 13, 2023
gregw added a commit that referenced this issue Dec 14, 2023
* Call ServletChannelState.asyncFailure from error listener. Fix #10933
* Separate invokers for read side and write side
* document async error issues
* updates from review
* updates from review
@sbordet
Copy link
Contributor Author

sbordet commented Dec 15, 2023

Fixed by #10949.

@sbordet sbordet closed this as completed Dec 15, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 12.0.5 - FROZEN Dec 15, 2023
sbordet added a commit that referenced this issue Dec 17, 2023
Most often failures come from the read side, so failure listeners are now serialized in the _readInvoker.
This avoids that a failure while parsing a request (e.g. an early EOF) results in concurrent executions of the invokeOnContentAvailable task and the invokeOnFailureListeners task.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Dec 17, 2023
Fixed HttpChannelTest.testOnFailure().

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
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
No open projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants