Jetty-12 Remove usage of HandlerList and reduce usage of Handler.Collection

[gregw]

"The best part is no part" - Elon Musk!

The overwhelming usage of HandlerList and Handler.Collection was for adding the DefaultHandler after the main handler. This PR adds a getter/setter for a DefaultHandler on the server, so we no longer need to always create a Handler.Collection structure. This has allowed the deprecated HandlerList and HandlerWrapper classes to be removed.

In implementing this PR, several problems were found in the calculation of InvocationType, not least that it was assumed that an empty Handler.Collection was BLOCKING. When this issue was fixed, any dynamic addition of contexts (deployer or SPI server) failed as the InvocationType changed. So this PR also introduces the isDynamic() attribute of all Handler.Containers. A dynamic container will always return BLOCKING from getInvocationType(), as there is always a race with a new handler being added. A non-dynamic container will return a real InvocationType, calculated from its children, but it's mutator methods will ISE if contained handlers are changed.

[gregw]

@sbordet @janbartel @joakime This is only a draft PR, but I would appreciate some early review to see if this is a viable approach.

I suggest starting with jetty-core/jetty-deploy/src/test/resources/jetty.xml to see the essence of this PR.

Then look at jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Handler.java to see the dynamic status I added to better deal with the InvocationType complications.

[gregw]

@olamy this PR updates a lot of the integration test XMLs. I think a lot of them were a bit wrong with strange structures of HandlerList and ContextHandlerCollections. I think I've fixed most of them up, but would be good if you could double check that I've not thwarted the purpose of any test.

[janbartel]

Couple of niggles and comments.

[lorban]

I like the idea of treating the DefaultHandler specially, like the ErrorHandler. This helps making the XML configs simpler.

But this opens the door to oddities which make me wonder if the DefaultHandler functionalities shouldn't be provided by the Server instead if allowing to configure a special handler?

[joakime]

I like the idea of treating the DefaultHandler specially, like the ErrorHandler. This helps making the XML configs simpler.

But this opens the door to oddities which make me wonder if the DefaultHandler functionalities shouldn't be provided by the Server instead if allowing to configure a special handler?

In Jetty 7 thru 11 ...

The various manipulations of DefaultHandler seen in the community are (in this order) ...

1. Remove it from the handler tree entirely (most common, either intentionally, or accidentally)
2. Configuring it (eg: setting showContexts to false, which is a straight 404 with no response body)
3. Replacing it with a RewriteHandler that just sends a redirect to a configured location.
4. Replacing it with their own custom implementation.

I'd be curious to know what would happen if the DefaultHandler is present in the Server.setDefaultHandler() and the handler tree.

[gregw]

@joakime

I'd be curious to know what would happen if the DefaultHandler is present in the Server.setDefaultHandler() and the handler tree.

Essentially this change makes Server a Handler.Collection of 2 Handlers. If a user sets up a structure with aDefaultHandler in it, then it will work perfectly fine and the server's default would just never get called.

[gregw]

@sbordet

Feels weird that we serve jetty-dir.css from ResourceHandler, but favicon.ico from the DefaultHandler, given the files are actually in the same place.

The two are different. We want jetty-dir.css to appear in every directory in the content tree, whilst favicon.ico is only in the top level, outside of any context.
Furthermore, jetty-dir.css is necessary for a feature we offer: listing directories, whilst favicon.ico` is a marketing stunt of dubious value.

We could definitely remove favicon.ico and nobody would care/complain and many would be happy to not have to turn it off!

[joakime]

We could definitely remove favicon.ico and nobody would care/complain and many would be happy to not have to turn it off!

I see a FavIconHandler as an option.

[gregw]

@joakime @sbordet @lorban Thanks for the review. Summary of updates:

• doco suggestions all accepted
• Server now does not create a DefaultServlet by default, so embedded usage remains unchanged.
• If users add a DefaultServlet with a Handler.Collection like before, everything will work normally
• Better examples of how we want the defaults server configured now.

[gregw]

I see a FavIconHandler as an option.

I don't think so. It's just a bit of cheap marketing, so nobody is ever going to add it deliberately. We are just using our default server setup with an incomplete user provided configuration (no root context) to: a) be helpful with a list of contexts; b) sneak in a little jetty promo.

Both are entirely optional, with b) being border line always unnecessary.

It is now off by default in an embedded usage and only triggers in jetty-home usage if there is no root, plus it is now easier to turn off with a property.

[sbordet]

A few more nibbles.

[gregw]

@sbordet I can't work out what your "This was not applied?" comment applies to? It is listed against outdated code?

[gregw]

@lorban Can you give me more details of what you want or dismiss your review?

[lorban]

@gregw I quite like how things turned:

• not needing to explicitly configure the default handler in every XML config (nice cleanup)
• having the possibility to configure a default handler on the server (nice addition)
• no default handler configured on the server by default (non-surprising default)

so it all LGTM.