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

Jetty 12 - Improve ErrorProcessor to handle error pages #8726

Closed
sbordet opened this issue Oct 18, 2022 · 4 comments · Fixed by #8736 or #9090
Closed

Jetty 12 - Improve ErrorProcessor to handle error pages #8726

sbordet opened this issue Oct 18, 2022 · 4 comments · Fixed by #8736 or #9090
Assignees
Labels
Bug For general bugs on Jetty side Stale For auto-closed stale issues and pull requests

Comments

@sbordet
Copy link
Contributor

sbordet commented Oct 18, 2022

Jetty version(s)
12+

Description
It would be nice if ErrorProcessor (or a subclass) can be configured with a mapping between the HTTP status code and an error page, something like:

errorProcessor.addPage(404, "/404.html");
@sbordet sbordet added the Bug For general bugs on Jetty side label Oct 18, 2022
@gregw
Copy link
Contributor

gregw commented Oct 18, 2022

In order to do this, we would need an ErrorPageErrorProcessor extends ErrorProcessor that is aware of the server (or perhaps context). Either way, there are two ways to implement:

  1. On every error process call the server.handle method is called to get a processor for the error page. This means there is a possible blocking call to Handler.handle before the processor is found.

  2. During onStart, fake requests are used to call the server.handle method to pre-obtain processors for the error pages. This would avoid the blocking stage during process, but I'm not sure it would work for all processors... as the requests ultimately passed to the processor would be different instance to the request passed to obtain the processor. Any processor that retains the initial request (possibly wrapped) would not work.

So I think 2. is a non-starter and probably an over optimisation anyway. 1 it is then.

@sbordet
Copy link
Contributor Author

sbordet commented Oct 19, 2022

@gregw left a feedback on the PR.

gregw added a commit that referenced this issue Oct 19, 2022
 + renamed by review
 + added simple test
gregw added a commit that referenced this issue Oct 20, 2022
 + renames by review
 + added more tests
@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 Oct 20, 2023
Copy link

This issue has been closed due to it having no activity.

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 Stale For auto-closed stale issues and pull requests
Projects
None yet
3 participants