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

client: Allow AsyncContentListener.onContent to throw checked Exceptions #11800

Closed
mperktold opened this issue May 16, 2024 · 2 comments · Fixed by #11825
Closed

client: Allow AsyncContentListener.onContent to throw checked Exceptions #11800

mperktold opened this issue May 16, 2024 · 2 comments · Fixed by #11825
Assignees

Comments

@mperktold
Copy link

Jetty version(s)
Jetty 12.x

Enhancement Description
AsyncContentListener implements ContentSourceListener.onContentSource and delegates to its own hook called onContent. Exceptions (or any Throwable really) thrown from onContent are catched and handled inside AsyncContentListener.onContentSource.

Now I have an implementation of AsyncContentListener.onContent that writes the content to a file, which can fail with an IOException. I would like to let these exceptions bubble up to onContentSource, which would handle it nicely. But this is not possible since IOException is a checked exception.

There are two workarounds:
One is to wrap the IOException in a RuntimeException and before rethrowing it.
The other is to completely handle the exception myself, basically copying the catch block from AsyncContentListener .onContentSource.

@mperktold
Copy link
Author

The same would apply to ContentListener, which in implements AsyncContentListener.onContent and delegates to its own onContent hook.

Here, I see that the default implementation indeed catches the exception and just aborts the response. AsyncContentListener.onContentSource would also notify the content source about the failure, but the source isn't available in onContent, so that's not possible there. Is that a problem?

If so, that would be a strong argument for allowing these hooks to throw checked exceptions, such that the failure can be properly detected and forwarded to the source. Otherwise, I guess catching the exception and aborting the response manually could be acceptable as well.

@lorban
Copy link
Contributor

lorban commented May 21, 2024

I've created a PR to modify the ContentListener and AsyncContentListener contracts to allow throwing Exception as that would be safe and backward compatible: #11825

The fact that ContentListener does not fail the source would still make onContent be called for all remaining contents despite the fact that the response was aborted. While correct, I believe a more sane default would be to fail the original Content.Source so that the flow of content would stop, so the above PR changes that too.

Thanks for the suggestion!

lorban added a commit that referenced this issue May 21, 2024
lorban added a commit that referenced this issue May 21, 2024
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue May 22, 2024
…1825)

#11800 allow ContentListener and AsyncContentListener to throw Exception

Signed-off-by: Ludovic Orban <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants