Possible regression in ContextFactory.getObjectInstance() between Jetty 11.0.22 and 12.0.11

[erdi]

Jetty version(s)

12.0.11

Jetty Environment

ee9

Java version/vendor (use: java -version)

Issue is not specific to java version used but it's openjdk 21.0.4.

OS type/version

Issue is not specific to OS used but it's macOS Sonoma 14.5

Description

I'm trying to update our apps from Jetty 11.0.22 to 12.0.11. After upgrading, I've noticed that WebAppContexts in our apps no longer correctly start up.

Each of our apps has multiple org.eclipse.jetty.server.Servers, each one of them having a WebAppContext wrapped in a StatisticsHandler wrapped in a RewriteHandler (even thought the fact that they are wrapped doesn't really matter here). The first WebAppContext starts ok but the subsequent ones fail with:

javax.naming.NamingException: This context is immutable; remaining name 'env'
	at org.eclipse.jetty.jndi.NamingContext.createSubcontext(NamingContext.java:355)
	at org.eclipse.jetty.jndi.NamingContext.createSubcontext(NamingContext.java:393)
	at org.eclipse.jetty.ee9.plus.webapp.EnvConfiguration.createEnvContext(EnvConfiguration.java:243)
	at org.eclipse.jetty.ee9.plus.webapp.EnvConfiguration.preConfigure(EnvConfiguration.java:80)
	at org.eclipse.jetty.ee9.webapp.Configurations.preConfigure(Configurations.java:496)
	at org.eclipse.jetty.ee9.webapp.WebAppContext.preConfigure(WebAppContext.java:492)
	at org.eclipse.jetty.ee9.webapp.WebAppContext.doStart(WebAppContext.java:527)
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:93)
	at org.eclipse.jetty.util.component.ContainerLifeCycle.start(ContainerLifeCycle.java:169)
	at org.eclipse.jetty.util.component.ContainerLifeCycle.doStart(ContainerLifeCycle.java:113)
	at org.eclipse.jetty.server.Handler$Abstract.doStart(Handler.java:491)
	at org.eclipse.jetty.server.handler.ContextHandler.lambda$doStart$0(ContextHandler.java:732)
	at org.eclipse.jetty.server.handler.ContextHandler$ScopedContext.call(ContextHandler.java:1244)
	at org.eclipse.jetty.server.handler.ContextHandler.doStart(ContextHandler.java:732)
	at org.eclipse.jetty.ee9.nested.ContextHandler$CoreContextHandler.doStart(ContextHandler.java:2695)
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:93)
	at org.eclipse.jetty.util.component.ContainerLifeCycle.start(ContainerLifeCycle.java:169)
	at org.eclipse.jetty.util.component.ContainerLifeCycle.doStart(ContainerLifeCycle.java:120)
	at org.eclipse.jetty.server.Handler$Abstract.doStart(Handler.java:491)
	at org.eclipse.jetty.server.handler.StatisticsHandler.doStart(StatisticsHandler.java:66)
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:93)
	at org.eclipse.jetty.util.component.ContainerLifeCycle.start(ContainerLifeCycle.java:169)
	at org.eclipse.jetty.util.component.ContainerLifeCycle.doStart(ContainerLifeCycle.java:120)
	at org.eclipse.jetty.server.Handler$Abstract.doStart(Handler.java:491)
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:93)
	at org.eclipse.jetty.util.component.ContainerLifeCycle.start(ContainerLifeCycle.java:169)
	at org.eclipse.jetty.server.Server.start(Server.java:624)
	at org.eclipse.jetty.util.component.ContainerLifeCycle.doStart(ContainerLifeCycle.java:120)
	at org.eclipse.jetty.server.Handler$Abstract.doStart(Handler.java:491)
	at org.eclipse.jetty.server.Server.doStart(Server.java:565)
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:93)

One notable fact is that as part of application startup some JNDI lookups are performed using the application classloader as the context classloader which is the parent of the web app classloaders used by WebAppContext.

So what is happening on 12 is:

• JNDI lookup with application classloader as the context classloader happens and an entry for that classloader is added to ContextFactory.__contextMap
• EnvConfiguration.createEnvContext() with a web app class loader as the context classloader is called and the subcontext is added to the context associated with application classloader because it's the parent of the web app classloader and a context was registered for it in the previous step
• PlusConfiguration.postConfigure() comes and calls PlusConfiguration.lockCompEnv() for the first WebAppContext which locks the context associated with the application classloader
• EnvConfiguration.createEnvContext() is called for the second WebAppContext and explodes with the above exception

This all worked fine on 11 and fails on 12. I tracked it down to this particular change (removal of the lines I'm linking to in the diff): https://github.com/jetty/jetty.project/pull/10897/files#diff-97c1c20fb48f65bd393d8d945d6c1ea54e39527ca7bf5731b2bbe297205e78f7L111-L128. It seems to have been part of the 12.0.5 release. So I believe that the reason why this works on 11 is that on 11 a new context is created for each of the WebAppContext's web app classloaders despite there being a context associated with their parent classloader. Interestingly the change in which the removed code was added in 2013 mentions multiple webapps and NamingException: ecd687d.

It still might be the case that this is not a regression but there is something inherently incorrect with our setup. Please advise if that's the case and how to rectify it.

How to reproduce?

Hopefully this is enough information but I can work on providing a self contained reproducible sample project. I would like to have it confirmed that it will be useful, though, before committing to the effort.

[joakime]

I have to ask.

Why do you have multiple Server instances in the same JVM?

[janbartel]

@erdi how are you deploying this, is it in the distro or as an embedded application? The exception you describe should really only happen if the webapps are sharing the same classloader, which they should not be. Are you certain that this is the only error, and there's not another exception earlier on in the deployment?

A small repro test case would be helpful, as obviously we have checked that deploying multiple webapps to the same Server and ee environment are compartmentalized as expected, but I'd need to understand how you're configuring your multiple servers and webapps.

[janbartel]

@erdi you say:

One notable fact is that as part of application startup some JNDI lookups are performed using the application classloader as the context classloader which is the parent of the web app classloaders used by WebAppContext.

Where/how are you doing "java:comp" lookups before the webapps have been deployed? What do you mean by "application classloader"? Can you please show me an example? In fact, I would really like the smallest repro test case to understand exactly what is happening.

[erdi]

I have to ask.

Why do you have multiple Server instances in the same JVM?

I'll gladly answer, if you have to ask, @joakime. 🙂

We use multiple Server instances because we expose different contexts on different ports. This allows us to make sure that other apps and external clients have only access to the features they are supposed to have access to and we can enforce that at the network level by only allowing access to certain ports from where these apps and clients are expected to be located at.

[erdi]

Thanks for being so responsive, @janbartel.

how are you deploying this, is it in the distro or as an embedded application?

It's an embedded application, we are using jetty classes in our code.

The exception you describe should really only happen if the webapps are sharing the same classloader

Their classloaders are sharing a common parent classloader (which is the application/system classloader) and all classes are loaded by that classloader.

Are you certain that this is the only error, and there's not another exception earlier on in the deployment?

I'm fairly certain that this is the exception causing the apps to stop from starting. That's because the apps start up correctly if I wrap the code performing the JNDI lookup using application/system classloader with creating a temporary classloader using application/system classloader as the parent, setting that temporary classloader as the thread context classloader and only then performing the JNDI lookup.

Where/how are you doing "java:comp" lookups before the webapps have been deployed?

We are using Guice injectors as the backing for web app contexts. As part of instantiation of one of the instances managed by the injector a method from RESTEasy is called which performs a JNDI lookup. This happens before calls to org.eclipse.jetty.server.Server.doStart() from the stacktrace in issue description are performed.

What do you mean by "application classloader"?

Using that wording as described here: https://www.baeldung.com/java-classloaders#classloader-types

A small repro test case would be helpful, as obviously we have checked that deploying multiple webapps to the same Server and ee environment are compartmentalized as expected, but I'd need to understand how you're configuring your multiple servers and webapps.

In fact, I would really like the smallest repro test case to understand exactly what is happening.

I will gladly start working on providing you with a project showing the issue now, given you are such an engaged maintainer, @janbartel. Hopefully I will have something ready by the end of the working day and if not then I will update you about the progress.

[joakime]

We use multiple Server instances because we expose different contexts on different ports.

That can be entirely handled via the Named connectors and virtualhosts @name behaviors that has been built into Jetty since Jetty 7.0
You do not need separate Server instances to do that.

See: https://github.com/jetty/jetty-examples/blob/12.0.x/embedded/connectors/src/main/java/examples/ConnectorSpecificContexts.java

[erdi]

That can be entirely handled via the Named connectors and virtualhosts @name behaviors that has been built into Jetty since Jetty 7.0
You do not need separate Server instances to do that.

Thanks for letting me know, this is definitely a much less clunky solution than using multiple servers for that. I don't believe that using multiple servers is the cause of this issue, though.

[erdi]

It turns out that creating a reproducer project was much easier than anticipated, @janbartel: https://github.com/erdi/jetty-12094. There is a README.md in the root of that repo which lists reproduction steps.

Please keep in mind that this was quickly put together so it's not cleanest of codes and a lot of it might look artificial. This is just the smallest possible reproducer I could quickly put together for the issue that I'm seeing in my project.

[erdi]

Oh, if there are any questions about the reproducer then please do let me know, I will happily answer.

[janbartel]

@erdi thanks for that repro, that clarified things. In jetty-12, we have 1 more classloader in the hierarchy as opposed to previous versions of jetty. Before it was the system classloader, the app classloader and then the webapp classloader. Now we have the system classloader, the app classloader, the EE environment classloader, and then the webapp classloader. If I create this environment classloader, your repro starts to work:

        URLClassLoader environmentLoader = new URLClassLoader(new URL[]{}, Thread.currentThread().getContextClassLoader());
        Thread.currentThread().setContextClassLoader(environmentLoader);
        new InitialContext().lookup("java:comp");

The lines that you saw changed in the ContextFactory were to take account of this new layer of classloading. I will ponder your situation further, but I don't immediately see how we can accommodate both styles of classloader hierarchy (ie pre jetty-12 and jetty-12) with the same ContextFactory code.

[erdi]

Thanks Jan. As alluded to in:

I'm fairly certain that this is the exception causing the apps to stop from starting. That's because the apps start up correctly if I wrap the code performing the JNDI lookup using application/system classloader with creating a temporary classloader using application/system classloader as the parent, setting that temporary classloader as the thread context classloader and only then performing the JNDI lookup.

I figured out the workaround you mentioned but only applied it at the lookup site rather than globally to reproduce this EE environment classloader because I wasn't aware that it's a thing. I considered applying it now globally but I can see that in #12107 you are already working on reverting the change I mentioned in the description of this issue. I can that the PR is targeted for 12.0.13 and you have a monthly release cadence so it should be available for me at the beginning of September. I'm not pressed for time with this so I will just wait.

Thanks again for being so responsive on this.

[robinvanpraet]

This change added an explicit require statement to jetty jndi in the module-info. A dependency that has been excluded by Spring Boot Starter Jetty for a long time. This means that a Spring Boot project, using Jetty, cannot start anymore using the Java Module system. I think the require statement should have been a require static.

[joakime]

@robinvanpraet opened issue ...

• Review requires org.eclipse.jetty.jndi; in jetty-ee10/jetty-ee10-plus/src/main/java/module-info.java #12422

[janbartel]

@robinvanpraet how long has org.eclipse.jetty.jndi been excluded by Spring Boot? Previous versions of jetty-plus module-info.java have had requires org.eclipse.jetty.jndi; - for example see jetty-11. For jetty-12 we refactored the jndi code to live in a core module, rather than have duplicate code in each eeX module. When fixing the problem above, we needed to explicitly call a class from this core jndi module inside the EnvConfiguration class, which (re)introduced the dependency on core jndi module. I'm dubious that this could be a require static, because why would you be enabling the jetty-ee10-plus module if you are not using the EnvConfiguration class or the PlusConfiguration class? In fact the service loader inside the jetty-ee10-plus will automatically place those classes on the list of configurations that get executed when an ee10 webapp starts. I'll make this same comment over on the new issue.