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

Fix/jetty 12 restore ee n fcgi #9796

Merged
merged 8 commits into from
Jun 5, 2023
Merged

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented May 25, 2023

Restored ee9 and ee10 FastCGI proxying to be used on the websites.

@sbordet sbordet requested review from gregw and joakime May 25, 2023 06:15
@sbordet
Copy link
Contributor Author

sbordet commented May 25, 2023

@gregw DistributionTests.testEEFastCGIProxying() fails for ee10 due to classloading issues.

The problem is that by the time we try to load the ee10 FastCGIProxyServlet from the XML files, the thread context classloader is not set to the environment classloader, but it is still the server classloader.

This does not happen in ee9 because ee9.nested.ContextHandler.CoreContextHandler.doStart() correctly sets the thread context classloader to be the environment classloader before calling super.doStart().
If I remove that line, then ee9 fails in the same way ee10 fails.

@sbordet
Copy link
Contributor Author

sbordet commented May 25, 2023

@gregw when you can please review bf92ee0, that is supposed to fix the ee10 classloader issue discovered in this PR.

@sbordet
Copy link
Contributor Author

sbordet commented May 25, 2023

Okay, my classloader changes just broke a large number of tests.
We need to discuss this.

@gregw
Copy link
Contributor

gregw commented May 26, 2023

Can you summarize what you are trying to do in the fix?

@sbordet
Copy link
Contributor Author

sbordet commented May 26, 2023

During deployment, the ContextHandler subclass is created by parsing the XML.

Just before that, we know the environment classloader so I thought that it must be set as threadContextClassLoader -- before this was done only if it was a core environment.

After the creation of the instance, I now set the environment classloader as the classloader of the ContextHandler subclass, otherwise no other code does (unless it's a WebAppContext).

Eventually, this ContextHandler subclass (with its child component tree) is started, which may cause the loading of a Servlet whose class is in the environment classloader.

For ee9, this works because of the code in ee9.nested.ContextHandler.CoreContextHandler.doStart().

For ee10 it does not work because ContextHandler.doStart() relies on doScope() which uses the ContextHandler.classLoader as the threadContextClassLoader, but unfortunately before this change it is still set to the server classloader, not the environment classloader.

So this fix is an attempt to set the ContextHandler.classLoader once, correctly, at creation time.
WebAppContext is a bit special in that it would have to take what set by the deployer and use it as the parent for the webapp classloader but that seems to be done correctly.

@gregw
Copy link
Contributor

gregw commented May 31, 2023

@sbordet if you are using servlets, then you should be using a ServletContextHandler, rather than a plain ContextHandler.
This does:

        getContext().call(() ->
        {
            // defers the calling of super.doStart()
            startContext();
            contextInitialized();
        }, null);

The servlets should be initialised with the classloader set inside startContext

So perhaps the problem is that the classloader itself has not been set to the environment classloader?

@gregw
Copy link
Contributor

gregw commented May 31, 2023

@sbordet perhaps the fix is to add the following to the constructor of the ServletContextHandler:

        if (this.getClass().getClassLoader() != Server.class.getClassLoader())
            setClassLoader(this.getClass().getClassLoader());

or maybe something like that can be done in the base ContextHandler#getClassLoader?

Ultimately, a context handler that has been loaded from an environment loader should start with that loader as it's classloader.

…oader' into 'fix/jetty-12-restore-eeN-fcgi'.
@sbordet sbordet merged commit 22641f1 into jetty-12.0.x Jun 5, 2023
@sbordet sbordet deleted the fix/jetty-12-restore-eeN-fcgi branch June 5, 2023 14:50
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.

2 participants