Various cleanups of Handler.insertHandler

[gregw]

Fixes #9297

[joakime]

When you run the EE10 ServletContextHandler.testReplaceHandler() in this branch you get a new WARN logging event. (should probably rename this specific test case, as we don't really do "replace" anymore in that test case)

2023-10-26 10:56:49.405:WARN :oejes.ServletContextHandler:main: ServletContextHandler.setHandler should not be called directly. Use insertHandler or setSessionHandler etc.

Looks like Handler.insertHandler() winds up calling ServletContextHandler.setHandler() which triggers this warning.

That's a bit of a confusing warning.

[sbordet]

Also, ServletContextHandlerTest.testReplaceHandler() fails if the ServletContextHandler in that test is configured with sessions or security.

What's the right semantic for ServletContextHandler.insertHandler()?

Given SCH -- [SessionH -- SecurityH -- ServletH], then insertHandler() of H -- X -- Y should result in SCH -- H -- X -- Y -- [SessionH -- SecurityH -- ServletH]?

And if you insertHandler() again with Z, is it after Y, or before H?

If that's the case, can you write that example in the javadocs of the overridden method?

[gregw]

@sbordet @joakime I can't get that failure locally? I've made some changes, but it is still a bit strange. I've punted to the next release cycle.

[gregw]

@sbordet @joakime nudge

[gregw]

@sbordet any chance of a re-review before you go on vacation, now that I've explained how core 2 nested Handlers work?

[sbordet]

@gregw I am still a bit confused by SCH interaction with setHandler() and insertHandler().

I would like the following:

• Make ServletHandler extends Handler.Abstract so that it's clear that it has no child. It still can return false, only in particular cases where users configure it to not have a DefaultServlet (in which case we just return the "core" 404).
• SCH.setHandler() always sets the first child after SCH (like all Singleton do), overwriting the existing one, with the special handling for "the triplet", i.e. Session|Security|ServletHandler, and relinks the triplet.
• The triplet is always last in the chain, so SCH.getTail() always returns the ServletHandler.

Some example:

sch.setHandler(gzipH);
sch.insertHandler(wsUpgradeH); // => sch-wsuh-gziph-[sessionh]-[securityh]-servleth
sch.getHandler() == wsuh
sch.getTail() == servleth

With the current PR, the code above sets up the following chain, which I think is wrong: sch-wsuh-[sessionh]-[securityh]-servleth-gziph, as we would never gzip.

With the proposal above:

sch.setHandler(gziph);
sch.setHandler(wsuh); // sch-wsu-[sessionh]-[securityh]-servleth -- gziph is lost as expected.

sch.setHandler(B);
sch.getHandler() == B; // The current PR breaks this.

I am sure the proposal above is a change to what I said earlier, but I find it clearer.
Would it work for you?

[joakime]

A quick test of this PR and it's handler tree looks like this ...

ServletContextHandler servletContextHandler = new ServletContextHandler(ServletContextHandler.SECURITY);

ServletHolder sh = new ServletHolder(new TestServlet());
servletContextHandler.addServlet(sh, "/foo");

servletContextHandler.setHandler(new GzipHandler());
servletContextHandler.insertHandler(new XYHandler());

Handler handler = servletContextHandler.getHandler(); // what does this look like ?

This results in the Handler tree ...

XYHandler 
   .handler = ConstraintSecurityHandler
      .handler = ServletHandler
          .handler = GzipHandler

Seems odd to have GzipHandler under the ServletHandler like this.
Would have expected it before the ConstraintSecurityHandler.

[gregw]

@joakime the following code:

        Server server = new Server();
        ServletContextHandler servletContextHandler = new ServletContextHandler(ServletContextHandler.SECURITY);
        servletContextHandler.setHandler(new GzipHandler());
        servletContextHandler.insertHandler(new Handler.Wrapper());

        server.setHandler(servletContextHandler);
        server.start();
        servletContextHandler.dumpStdErr();

produces:

oeje10s.ServletContextHandler@740fb309{ROOT,/,b=null,a=AVAILABLE,h=oejs.Handler$Wrapper@7bd7d6d6{STARTED}} - STARTED
+= oejs.Handler$Wrapper@7bd7d6d6{STARTED} - STARTED
|  += oeje10ss.ConstraintSecurityHandler@28cda624{STARTED} - STARTED
|     += oeje10s.ServletHandler@4dc27487{STARTED} - STARTED
|     |  += GzipHandler@6a4f1a55{STARTED,min=32,inflate=-1} - STARTED

I think the insert behaviour is exactly what we want.
I agree it is a little strange that SCH.setHandler does some magic so that the GzipHandler is after the ServletHandler, but that is what @sbordet asked for. I'm a 60:40 for the previous behaviour where setHandler was always a set on the current handler with minimal magic. But there always was a little bit of magic and sometimes warnings.... so this approach just embraces the magic and avoids the warnings.

Either way, I don't think it is hugely important, as I can't see I real use case for wanting to do setHandler in this situation. However, with EE10 and beyond, it is slightly more important as users can no longer use a ServletHandler with a plain ContextHandler. It only works with a ServletContextHandler (to avoid the need for a ScopedHandler mechanism we establish servlet scope as we enter the context).

As I said, I'm 60:40... but as I've currently implemented/tested the 40... I'm happy to go with it if there is consensus???

[gregw]

@sbortdet

@gregw I am still a bit confused by SCH interaction with setHandler() and insertHandler().

I think we have always had some strangeness here where one handle carries a tuple of other handlers that must be in a specific structure. I'm not sure we can avoid t he situation. We can only address confusion by a bit better consistency and better documentation, which is what I think this PR does. We can't make the issue go away.

I would like the following:

• Make ServletHandler extends Handler.Abstract so that it's clear that it has no child. It still can return false, only in particular cases where users configure it to not have a DefaultServlet (in which case we just return the "core" 404).

@sbordet there are definitely usages of ServletHandler as a wrapper (see below). This is not an option.

• SCH.setHandler() always sets the first child after SCH (like all Singleton do), overwriting the existing one, with the special handling for "the triplet", i.e. Session|Security|ServletHandler, and relinks the triplet.

That's what I've now implemented for you. If it is not a Session|Security|ServletHandler, it always sets the handler of the ServletHandler, overwriting the existing one. So it is setting the first child after SCH.

• The triplet is always last in the chain, so SCH.getTail() always returns the ServletHandler.

Not an option. There are valid use-cases for falling through a ServletHandler. For example we can put a ResourceHandler after the ServletHandler and not have a DefaultServlet(with all it's complexities).

Some example:

sch.setHandler(gzipH);
sch.insertHandler(wsUpgradeH); // => sch-wsuh-gziph-[sessionh]-[securityh]-servleth
sch.getHandler() == wsuh
sch.getTail() == servleth

With the current PR, the code above sets up the following chain, which I think is wrong: sch-wsuh-[sessionh]-[securityh]-servleth-gziph, as we would never gzip.

I said I was 60:40 in my reply to @joakime, but now I'm 95:5 for the former behaviour of setHandler with less magic since the following should always be true:

    sch.setHandler(xyzHandler);
    sch.getHandler() == xyzHandler

I think the less magic, then the less confusion. My intention is to go back to a simple setHandler with warnings for bad behaviours. Perhaps it should not even intercept Session|Security|ServletHandler

[gregw]

@joakime nudge

[gregw]

@sbordet nudge

[sbordet]

I am still confused.

I think we should stick to the behavior of the other Handlers and not make this one any special.

As I said, I was wrong wanting setHandler() to set the child of ServletHandler.
I think it should just do what any other Handler does, i.e. setting the immediate child.

insertHandler() would have the same semantic as others, i.e. setting the immediate child and shifting the current child.

The "triplet" always come last.

I'm fine for ServletHandler to be a Wrapper if there are use cases, but setting its child should always be done explicitly, i.e. sch.getServletHandler().setHandler(...), not via some "magic" in SCH.setHandler().

[sbordet]

I don't think this logic is right.
Calling setHandler() with one of the 3 handlers will never work.

[gregw]

It has been this way for several years at least. So we are not making it special. It is already special.
I'm not sure we want to put a breaking change into the API without thought. I found a few places in the code that did rely on this behaviour, so other users may also.

I think if we want to change setHandler, that should be a different PR with more consideration.

[gregw]

@sbordet I've put the implementation of setHandler back to exactly what it has been for some time. However, this PR removes any usage of it that depends on that behaviour. If it is an issue, then we can make the method always warn if called on SCH.

[sbordet]

@gregw see my last commit where I added in one place and moved in another the call to relinkHandlers().
LGTM with the current behavior.

[gregw]

@gregw see my last commit where I added in one place and moved in another the call to relinkHandlers(). LGTM with the current behavior.

@sbordet

The move of the relink in insertHandler is good, as it saves doing it twice.

But the relinkHandlers in setHandler is wrong (or perhaps the relinkHandlers impl is not good enough. If I try to do a set chain of handlers A->B->C->SessionHandler->ServletHandler, then relinkHandlers will look at A, see that its next handler is not SessionHandler and replace it, so the chain will end up A->SessionHandler->ServletHandler and B->C will be dropped.

I'll make a unit test to check this and then try to fix relinkHandler.... but I just don't think we should try to do magic with anything other than the Session/Security/Servlet handlers. Even that magic is probably too much.

[gregw]

@sbordet OK I was wrong. relinkHandlers is good enough to follow the chain. But it is just more magic. I don't like it, but OK.