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

Use HandlerList instead of HandlerCollection #4757

Merged
merged 2 commits into from
Apr 15, 2020

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Apr 8, 2020

The HandlerCollection class is both a concrete class and the base class for HandlerList and ContextHandlerCollection. Behaviours are:

  • HandlerCollection has a list of handlers that are called in order regardless of errors or handled status
  • HandlerList calls handlers in order until isHandled()==true or an exception
  • ContextHandlerCollection attempts to map a request to a handler by contextPath and virtual host and then calls matching in order until isHandled()==true` or an exception

Originally the basic jetty.xml setup was:

Server
  +-- HandlerCollection
       +-- ContextHandlerCollection
       +-- DefaultHandler

Which with some optional modules was extended to something like:

Server
  +-- HandlerCollection
       +-- RewriteHandler
       +-- ContextHandlerCollection
       +-- RequestLogHandler
       +-- DefaultHandler

So the HandlerCollection behaviour was needed so that the request was always rewritten and always logged. The DefaultHandler does it's own isHandled()==true check so it only handles unhandled requests.

However, now in Jetty-10 the extended structure is

Server
  +-- RewriteHandler
  |   +-- HandlerCollection
  |       +-- ContextHandlerCollection
  |       +-- DefaultHandler
  +-- RequestLog

So the HandlerCollection semantic is no longer needed. This PR replaces the common usage of HandlerCollection with the more intuitive HandlerList

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Copy link
Contributor Author

gregw commented Apr 8, 2020

I've given this a wide review list as I want knowledge of change this spread.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Copy link
Contributor Author

gregw commented Apr 14, 2020

@sbordet @joakime @janbartel nudge

@gregw
Copy link
Contributor Author

gregw commented Apr 14, 2020

@sbordet @joakime @janbartel nudge2

Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good.

But I would have liked it if you had left some of the HandlerList.addHandler(Handler) usages in place, as that's an important API that still needs to be tested properly.

Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would like to clarify the case where there is (ServletContextHandler, DefaultHandler).
If ServletContextHandler is a terminal Handler, then there is no point in having a DefaultHandler afterwards.
If ServletContextHandler can be configured to be non-terminal, why can it be configured so at all - seems counter intuitive that it can be both.

@gregw gregw requested a review from sbordet April 15, 2020 11:41
@joakime
Copy link
Contributor

joakime commented Apr 15, 2020

Is there a filed issue for this PR?

Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion with @gregw, LGTM.

@gregw gregw merged commit 2addb6a into jetty-10.0.x Apr 15, 2020
@gregw gregw deleted the jetty-10.0.x-HandlerList branch April 15, 2020 16:42
@joakime joakime changed the title Jetty 10.0.x use HandlerList instead of HandlerCollection Use HandlerList instead of HandlerCollection Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants