cometd / cometd

The CometD project, a scalable comet (server push) implementation for web messaging.
https://cometd.org
Apache License 2.0
569 stars 207 forks source link

Nashorn scripts being held through `inheritedAccessControlContext` of pooled threads #739

Closed szegedi closed 7 years ago

szegedi commented 7 years ago

Relates to #729.

Analyzing thread dumps after fixing #737 reveals that a typical strong path to a GC root for retained Nashorn script objects looks like this

That is, these scripts' class loader becomes part of the inheritedAccessControlContext of a Thread object. These threads have names like pool-2-thread-1 etc. Vast majority of them seems to be referring a script of the form http://localhost/$randomPort/cometd/js/dojo/window_location (also, without Dojo too for the jQuery test suite).

It appears that pooled threads are being created from within Nashorn script execution, so those threads will pin the Nashorn compiled script classes as part of their inherited access control context. I'm not sure where are these threads created, but maybe you should consider pre-populating the thread pools from outside Nashorn first.

szegedi commented 7 years ago

The threads appear to have java.util.concurrent.ThreadPoolExecutor$Worker instances as their targets, and seem to be managed by a j.u.c.ScheduledThreadPoolExecutor; that's all I can glimpse from the thread dump…

sbordet commented 7 years ago

Attila, this is awesome !

Those threads are created from browser.js that is the simulation of the browser environment. The ScheduledThreadPoolExecutor is used to implement the browser's setTimeout() and setInterval().

As you say, should be easy to create the scheduler outside and pass it through the bindings.

Thanks a ton !

sbordet commented 7 years ago

Attila, do I understand correctly that any Thread object created within the execution of a script will retain the script class loader via Thread.inheritedAccessControlContext ?

So it's not only matter of creating the thread pool outside the script, I must also pre-allocate the threads in the thread pool to avoid that a script ever creates a Thread object, right ?

szegedi commented 7 years ago

Yes, I believe this is the case.

Alternatively, there's j.u.c.Executors.privilegedThreadFactory() method, it'll create a ThreadFactory with ACC captured at that point and initialize all threads with it. I believe if you use it with your thread pool, it'll create threads with the ACC that was current at the invocation of privilegedThreadFactory, so you need not pre-populate the pool. It's also setting the current class loader to be the thread's context class loader, so it's actually doing more than we'd like, which might or might not have some unwatned side effects :-) Give it a try, though.

sbordet commented 7 years ago

Attila, many thanks for this ! Considering it fixed.