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 Http[Input|Output].Interceptor #6167

Closed
sbordet opened this issue Apr 13, 2021 · 8 comments
Closed

Review Http[Input|Output].Interceptor #6167

sbordet opened this issue Apr 13, 2021 · 8 comments
Assignees
Labels
Stale For auto-closed stale issues and pull requests

Comments

@sbordet
Copy link
Contributor

sbordet commented Apr 13, 2021

Jetty version
10.0.x

Description
Http[Input|Output].Interceptor are underspecified and need to be reviewed and clarified.

This issue stems from work on #6163 and also #6156 and #6010.

Interceptors are not allowed to throw checked exceptions, so they have to wrap failures in runtime exceptions, which would be unnecessary.

Furthermore, if an interceptor throws, the exception handling needs to be careful in both blocking and non-blocking I/O.

@lachlan-roberts @lorban @gregw please add your comments about this matter.

@sbordet
Copy link
Contributor Author

sbordet commented Apr 13, 2021

@lorban please use this issue as an umbrella to port #6163 to Jetty 10.

However, this issue is also for HttpOutput.Interceptor so you may want to issue a PR for HttpInput but not close this issue until also HttpOutput is done.

@lachlan-roberts
Copy link
Contributor

The HttpOutput.Interceptor can fail the callback instead of throwing, however failing the callback causes the connection to be aborted with no 500 response even if the HttpChannel is not committed.

@gregw
Copy link
Contributor

gregw commented Apr 14, 2021

An interesting question is that if there is a failure during an async write (either in interceptor or below) then should it be thrown from the call to async write? If we don't throw, then the caller might loop on isReady and write, never seeing the exception. We can't call onError because we are already in onWritePossible. @sbordet how sure are we that we have not introduced such a loop with the new catches?

So i think we should throw.... but then do we call onError as well once the call to onWritePossible returns?

@lorban lorban removed their assignment Apr 19, 2021
@lorban
Copy link
Contributor

lorban commented Apr 19, 2021

HttpInput side is done.

lachlan-roberts added a commit that referenced this issue Apr 30, 2021
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Apr 30, 2021
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Apr 30, 2021
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@github-actions
Copy link

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label Apr 20, 2022
@sbordet sbordet removed the Stale For auto-closed stale issues and pull requests label May 1, 2022
@github-actions
Copy link

github-actions bot commented May 2, 2023

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label May 2, 2023
@sbordet sbordet removed the Stale For auto-closed stale issues and pull requests label May 13, 2023
Copy link

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label May 13, 2024
@sbordet
Copy link
Contributor Author

sbordet commented May 15, 2024

Closing, as Jetty 12 does not have interceptors anymore.

@sbordet sbordet closed this as completed May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale For auto-closed stale issues and pull requests
Projects
None yet
Development

No branches or pull requests

4 participants