Fixes and extra testing for EE9 ContextHandler class loading

[lachlan-roberts]

The org.eclipse.jetty.ee9.nested.ContextHandler was not properly setting the ClassLoader in certain situations such as async dispatches and when the ContextHandler was not at the root of the ee9 Handler hierarchy.

I had to add code in ContextHandler.doStart() so that the CoreContextHandler and the ContextHandler are started together. If CoreContextHandler is already fully STARTED when the ContextHandler is starting, then certain operations will throw IllegalStateException.

This code for will only execute if the ContextHandler was not at the root of the EE9 Handler hierarchy and so we are not started by our CoreContextHandler. If this isn't the case we can just call doStartInContext as normal.

[lachlan-roberts]

the failing tests are passing locally for me even from command line, investigating now

[gregw]

I think you have more changes that have not been pushed?

[gregw]

This is good as is, but I think we need more.

After #9905 has been merged we need to check if AsyncListener#onError is called with the context set. Probably also good to check if onWritePossible and onDataAvailable callbacks have classloader set (or check if we already have tests for those). These things can be done after merging this PR.

[lachlan-roberts]

found some additional issues with this, still working on the fix

[gregw]

Just a quick request-changes response before a deep review. Can you consider the suggestion to see if it would be simpler, whilst I review the rest of the PR.

[gregw]

Just a thought... but would it be simpler to always have this handler as a non managed bean in the core context handler, and to always have a lifecycle listener registered that does the start and stop. That way this is the normal path rather than a transient strange path?

[gregw]

Note that if we did it this way, we'd need to explicitly dump the context handler from the core context handler, so that we would get the deep dump, but I think that is simple to do.

[gregw]

@lachlan-roberts thoughts?

[lachlan-roberts]

Sorry didn't see this comment.

I think even if we had the listener permanently registered we would still need this if statement to manually start the CoreContextHandler.

Something like this

if (org.eclipse.jetty.server.handler.ContextHandler.getCurrentContext() != _coreContextHandler.getContext())
     _coreContextHandler.start();
// else do nothing (don't call doStartInContext because listener will)

[lachlan-roberts]

And then would the listener lifeCycleStarting be called before or after this doStart? I'm not sure.

We would need to ensure this is in STARTING state when the listener is called (not just the CoreContextHandler in STARTING state).

[gregw]

We need some javadoc somewhere that describes how this Context can be invoked in 2 different ways:

1. via it's nested coreContextHandler which will wrap the core request/response and create the nest baseRequest/baseResponse
2. via a direct call to handle when this handler is nested within another context handler.

In fact, I really don't like it that this class can simultaneously be used in both ways. In the case of a nested contextHandler, that means there is a second coreContextHandler just doing nothing. I'd prefer to have a mode where the second coreContextHandler was never created and only then could it receive requests from handle.

At the very least, this needs to be javadoc'd, but I'd prefer a mode.... I don't think splitting is possible.

Perhaps the mode can be established by a call to the get of the coreContextHandler?

[lachlan-roberts]

I think the split is possible. Maybe if the CoreContextHandler had a nested ContextHandler and delegated to that instead of the other way around.

Right now nested ContextHandler delegates lots of its methods to CoreContextHandler and its ScopedContext. So hard to separate them without some more fundamental changes.

Anyway this would be for another PR if we decide to do it. For now I have added some javadoc to explain the two different cases how this can be invoked.

[gregw]

I'm still not a big fan of the ability to use this handler in two modes.

We should be able to detect if the coreContextHandler has been inserted in the handler tree or not. If it has been, then only it may be used to accept requests. If it has not been, then only the normal EE9 handle method may be used.

I don't want to hold this up any longer, so I will approve, but please open an issue to fix the above.... or better yet fix it now before merging.

[lachlan-roberts]

should be able to detect if the coreContextHandler has been inserted in the handler tree or not. If it has been, then only it may be used to accept requests. If it has not been, then only the normal EE9 handle method may be used.

I don't want to hold this up any longer, so I will approve, but please open an issue to fix the above.... or better yet fix it now before merging.

Not sure what is the fix you are asking for. The "normal EE9 handle" method is always used in both cases, so its not one or the other. The difference is it could be someone elses CoreContextHandler which brought us into EE9 land. But we also use the CoreContextHandler in both cases as well for its ScopedContext.

Anyway I will merge now and we can discuss the other fix you had in mind after.