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

Make Server::setDefaultHandler and Server::setErrorHandler arguments consistent #12802

Closed
scrat98 opened this issue Feb 18, 2025 · 3 comments · Fixed by #12837
Closed

Make Server::setDefaultHandler and Server::setErrorHandler arguments consistent #12802

scrat98 opened this issue Feb 18, 2025 · 3 comments · Fixed by #12837

Comments

@scrat98
Copy link
Contributor

scrat98 commented Feb 18, 2025

Jetty version(s)
12.0.16

Enhancement Description
There is an inconsistency in arguments between Server::setDefaultHandler and Server::setErrorHandler.

  • Server::setDefaultHandler accepts org.eclipse.jetty.server.Handler
  • Server::setErrorHandler accepts org.eclipse.jetty.server.Request.Handler

I would like to use in Application code only org.eclipse.jetty.server.Handler and if I do so and use it with Server::setErrorHandler then I'll get a warning
WARN o.e.jetty.server.Handler$Abstract - No Server set for csclhs.UnhandledErrorsHandler@5ace1ed4{STARTING} and due to this I need to implement my hanlder as a org.eclipse.jetty.server.Request.Handler to suppress this warning.

Nothing serious, but I think it should be easy to change.

@janbartel
Copy link
Contributor

@scrat98 I'm not sure it's a good idea to have a single handler that performs both error handling and also last resort handling of a request. I think it makes your code more monolithic and less flexible, but, ymmv.

As a org.eclipse.jetty.server.Handler is-a org.eclipse.jetty.server.Request.Handler, then you should be ok, assuming you're using the same instance for both setDefaultHandler and setErrorHandler and you call them in that order: the server will be set on the instance by setDefaultHandler so it's ready for the call to setErrorHandler. Are you in fact using different instances? If so, I think it's indicative that you should probably split your code. If you really don't want to/can't do that, then you'll have to call setServer on the instance before you pass it into setErrorHandler.

@scrat98
Copy link
Contributor Author

scrat98 commented Feb 28, 2025

@janbartel I'm using different instances (have ErrorsHandler and DefaultHandler classes). My point was that in the application code I have plenty of handlers (20 for example) and all of them implement server.Handler.

But to avoid warn log, only single ErrorHandler implements server.Request.Handler also with a comment if someone decided to change it to server.Handler. And this inconsistency just gives my eye no rest, nothing serious.

@janbartel
Copy link
Contributor

I think we will have to leave the published method signatures as they are - definitely at least for 12.0 - but maybe in setErrorHandler we could call setServer iff the handler isinstanceof org.eclipse.jetty.server.Handler. Let me take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants