Classloader leaks from ShutdownThread and QueuedThreadPool

[VasilVasilev93]

Jetty version
9.4.35.v20201120

Description
Hello, when running inside osgi environment, both classes allow classloader leaks. I noticed the leaks when using jetty's WebSocketClient from a bundle. After uninstall of the bundle, and of course cleaning up the WebSocketClient instance, its classloader remains referenced inside ShutdownThread's inheritedAccessControlContext protection domain.Locally I made a change inside ShutdownThread class, to create the thread inside a doPrivileged block, which fixed the issue.
As for QueuedThreadPool, when a thread from the pool is used by a bundle, after its uninstall, the thread's contextClassLoader remains the bundle classloader, which creates a leak, until the thread gets reused by some other bundle. I made a local change to the class, to reset the contextClassLoader after the thread is marked as idle, which fixed the issue.

[janbartel]

@VasilVasilev93 I believe the classloader pinning by the WebSocketClient instance is being fixed in PR #5840, but perhaps @lachlan-roberts can comment more about that?

As for the QueuedThreadPool contextClassLoader, I'll take a look at that, or we'd be happy to review a PR if you want to submit one?

[lachlan-roberts]

I don't think this is related to PR #5840, that PR is for jetty 10 and is the Javax client WebSocketContainer is being registered with the shutdown thread.

But in Jetty 9.4 the WebSocketClient is also registered with the shutdown thread by default. From the code it looks like the WebSocketClient will always deregister itself from the ShutdownThread in its doStop() method. So are you sure you are stopping your WebSocketClient instance in your cleanup?

You should be able to avoid any issues associated with the shutdown thread by explicitly calling setStopAtShutdown(false) before starting the WebSocketClient.

WebSocketClient client = new WebSocketClient();
client.setStopAtShutdown(false);
client.start();

[VasilVasilev93]

@lachlan-roberts Uhm, the problem is not related to how WebsocketClient is using the ShutdownThread, I just gave an example. In osgi environment, ShutdownThread class is not loaded until a call is made from a bundle, f.e. ShutdownThread.deregister()/register()/getInstance(). When instantiating the thread, the protection domain - inheritedAccessControlContext.context (ProtectionDomain[]) - contains the bundle class loader, as well as jetty-util's class loader, but when ShutdownThread instance is created inside a doPrivileged block, it contains only the class loader of jetty-util bundle

public class ShutdownThread extends Thread
{
    ...
    private static final ShutdownThread _thread = AccessController.doPrivileged(new PrivilegedAction<ShutdownThread>() {
      @Override
      public ShutdownThread run() {
        return new ShutdownThread();
      }
    });
    ....
}

[janbartel]

@VasilVasilev93 for the QueuedThreadPool: as a request handling thread drains out of a context, its context classloader is reset, so I'm not sure why the bundle classloader is still seen on the thread?

[VasilVasilev93]

@janbartel Is this happening somewhere outside the QueuedThreadPool class?

[janbartel]

@VasilVasilev93 yes, this happens in ContextHandler when a request is leaving a context, see: https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java#L1287

The above link is for jetty-10, but it's the same code for jetty-9.

It would be interesting if you could find out more about the thread in QueuedThreadPool that still had the classloader associated with it.

[VasilVasilev93]

@janbartel Thanks, I will check this out, and I will upload screenshots from eclipse memory analyzer later

[VasilVasilev93]

@janbartel I tested the above, it is working as you say, the classloader is reset indeed, but the issue here is that the thread is instantiated with the bundle's classloader and after the bundle gets uninstalled, the thread is still holding it.

[janbartel]

@VasilVasilev93 interesting. We will have to take a look in a non-osgi environment: presumably the thread's contextClassLoader would still be that of a context that had been undeployed ....

How are you running jetty with osgi? Are you starting an osgi container from with a webapp or are you starting an osgi container and then starting jetty within it?

[VasilVasilev93]

@janbartel Starting jetty within an osgi container.

[janbartel]

@VasilVasilev93 ah hah! There are circumstances in which a queued thread pool thread is inside a context (thus with that context's classloader set on it), and it then creates another thread pool thread because all threads are currently busy. According to the constructor of Thread, a thread will inherit the context classloader of the thread that created it - in this case it would be the context's classloader and voila, classloader leak!

The fix we're considering is to ensure that every thread that the QueuedThreadPool creates has it's classloader set explicitly to something safe like so:
(see https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java#L703)

    @Override
    public Thread newThread(Runnable runnable)
    {
        Thread thread = new Thread(_threadGroup, runnable);
        thread.setDaemon(isDaemon());
        thread.setPriority(getThreadsPriority());
        thread.setName(_name + "-" + thread.getId());
        thread.setContextClassLoader(QueuedThreadPool.class.getClassLoader());  // add this line!
        return thread;
    }

Is there any chance you could try this code out in your environment and report back if it solves the problem?

[VasilVasilev93]

@janbartel Yes, this is ok and it seems logical, I have it tested, it's working.

[janbartel]

@VasilVasilev93 excellent! I've raised #5894 with this fix.

I'll do a separate PR for the ShutdownThread.

[VasilVasilev93]

Okay, thanks :)

[janbartel]

@VasilVasilev93 re-reading the problem reported with the ShutdownThread, it seems to me that the QueuedThreadPool threads should also suffer from the retention of the inheritedAccessControlContext and it's associated ProtectionDomains, and hence the pinning of the contextClassLoader. Is this not the case?

[VasilVasilev93]

@janbartel In our specific case it doesn't cause issues, but yes, I believe so.

[janbartel]

Fixed via PR #5894