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

Create Dispatcher.Default threads with the same context classloader a… #3877

Merged
merged 3 commits into from
Nov 30, 2023

Conversation

qwwdfsad
Copy link
Contributor

@qwwdfsad qwwdfsad commented Sep 4, 2023

…s the dispatcher itself

In order to properly operate in modularized on a classloader level environments with the absence of other workarounds (i.e. supplying application-specific thread factory)

Fixes #3832

…s the dispatcher itself

In order to properly operate in modularized on a classloader level environments with the absence of other workarounds (i.e. supplying application-specific thread factory)

Fixes #3832
@qwwdfsad
Copy link
Contributor Author

qwwdfsad commented Sep 4, 2023

Unfortunately, no FieldWalker test for that because Java prohibits reflective access to java.* classes

@dkhalanskyjb
Copy link
Contributor

$ git grep isDaemon
kotlinx-coroutines-core/jvm/src/DefaultExecutor.kt:            isDaemon = true
kotlinx-coroutines-core/jvm/src/ThreadPoolDispatcher.kt:        t.isDaemon = true
kotlinx-coroutines-core/jvm/src/debug/internal/DebugProbesImpl.kt:        weakRefCleanerThread = thread(isDaemon = true, name = "Coroutines Debugger Cleaner") {
kotlinx-coroutines-core/jvm/src/scheduling/CoroutineScheduler.kt:            isDaemon = true
kotlinx-coroutines-core/jvm/test/knit/ClosedAfterGuideTestExecutor.kt:        isDaemon = true
kotlinx-coroutines-debug/src/junit/CoroutinesTimeoutImpl.kt:    val testThread = Thread(testResult, "Timeout test thread").apply { isDaemon = true }

We have several other places where we create daemon threads; isn't the fix also needed for them? What makes Dispatchers.Default special? Sure, it is a global singleton, but so is DebugProbesImpl.weakRefCleanerThread, and so can be, depending on the user's code, newFixedThreadPoolContext. What if someone accidentally creates the DefaultExecutor thread in an environment with its own classloader?

@qwwdfsad
Copy link
Contributor Author

qwwdfsad commented Sep 20, 2023

What if someone accidentally creates the DefaultExecutor thread in an environment with its own classloader?

That's totally okay and a supposed use pattern. E.g. having kotlinx-coroutines dependency in Gradle plugin typically means it's loaded by a custom classloader.

The problem is not about daemon threads or classloaders per see, but about a pretty specific case:

  1. The target in question should be a globally accessible application-level singleton. Control of everything else (e.g. newThreadPoolContext) and its semantics (e.g. user-defined newFixedThreadPoolContext) is users' consideration
  2. The target should create its own threads lazily from a thread different from where the target itself was initialized.

E.g. consider the following:

// T1, main app classloader
Dispatchers.Default.doSomething() 

Loads and initializes Dispatchers.Default object, its classloader is app classloader

// T2, app plugin classloader which is loaded and unloaded dynamically as a child of app classloader
Dispatchers.Default.dispatch(...) 

Here, due to the nature of our dispatcher, a new thread is spawned. By default, new thread inherits context classloader from the enclosing thread, which is, in our case, app plugin classloader.

After that, we have a situation where Dispatchers.Default that is meant to be shared across all "plugins" retains a reference to the child classloaders and prevents their unloading.

isn't the fix also needed for them?

Nope:

  • newFixedThreadPoolContext is a Java's executor with a supplied thread factory. We correctly inherit the semantics of java.lang.Executors.newScheduledThreadPool.
  • DefaultExecutor -- starts its thread in the same context where it is initiated (shutdown is used only in our own tests)
  • CoroutinesTimeoutImpl the thread is bound to the same context rule is

Does it make sense? Does it answer your question?

@dkhalanskyjb
Copy link
Contributor

dkhalanskyjb commented Sep 20, 2023

DefaultExecutor -- starts its thread in the same context where it is initiated

By "the same context", do you mean it's in the same thread? If so, I disagree.

The thread in DefaultExecutor starts when the thread variable is accessed, which may happen after DefaultExecutor already finished initialization. Running tests in a debugger, I see that the thread only gets created when dispatch or invokeOnTimeout is called on the DefaultExecutor.

The initialization of DefaultExecutor may happen much earlier. For example, it's possible to ensure its initialization by calling Dispatchers.IO.limitedParallelism(100). Then, by definition of LimitedDispatcher, it will access and initialize DefaultDelay. We clearly can call Dispatchers.IO.limitedParallelism(100) in another thread before any dispatches actually happen.

Also, what about weakRefCleanerThread? It does seem to tick both of the boxes you listed.

@qwwdfsad
Copy link
Contributor Author

qwwdfsad commented Sep 20, 2023

If so, I disagree.

You are right, this is my oversight, I was quite sure that ensureStarted is invoked immediately after the initialization. Thanks for the example with initialization, this is indeed the erroneous pattern.

Also, what about weakRefCleanerThread? It does seem to tick both of the boxes you listed.

I'm not really sure about this one.
On the one hand, this one has another property that it has to be launched (DebugProbes.install()) and shut down (DebugProbes.uninstall()) explicitly.

On the other hand, I can craft an example that leads to the leak, though it should be really obscure. (Initialize in one place, install in the other, then do not uninstall, expecting that the debugger will be shared across all the application).
I would prefer avoid touching it.

@qwwdfsad qwwdfsad merged commit a79db37 into develop Nov 30, 2023
1 check passed
@qwwdfsad qwwdfsad deleted the dispatched-fix-classloader-leak branch November 30, 2023 15:30
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.

None yet

2 participants