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

attach_current_thread_as_daemon (and any notion that we support 'daemon' threads) should probably be removed #469

Open
rib opened this issue Jul 6, 2023 · 0 comments

Comments

@rib
Copy link
Contributor

rib commented Jul 6, 2023

The low-level JNI API supports the idea that you can attach a thread to a JVM in such a way that it won't automatically get detached from the JVM when destroying the VM!

The high-level idea behind Java daemon threads is that applications could use them for work that they don't necessarily want to block the application from exiting (things that run continuously like watchdogs/loggers).

The problem with mapping that idea to JNI thread attachment though is that it leaves a dangerous race condition between destroying the VM and exiting the application + terminating the daemon threads. If any of those daemon threads do anything to interact with JNI / the VM they are going to hit undefined behaviour.

In the context of this crate which is trying to provide a (mostly) safe JNI API I really struggle to see any way in which daemon thread attachments can be exposed safely.

Since it's relevant here I'll copy docs I wrote for detach_current_thread which highlight how we currently require applications to explicitly detach daemon threads if they call JavaVM::destroy() to avoid this risk of undefined behaviour:

/// Explicitly detaches the current thread from the JVM.
///
/// _**Note**: This operation is _rarely_ appropriate to use, because the
/// attachment methods [ensure](#attaching-native-threads) that the thread
/// is automatically detached._
///
/// Detaching a non-attached thread is a no-op.
///
/// To support the use of `JavaVM::destroy()` it may be necessary to use this API to
/// explicitly detach daemon threads before `JavaVM::destroy()` is called because
/// `JavaVM::destroy()` does not synchronize and wait for daemon threads.
///
/// Any daemon thread that is still "attached" after `JavaVM::destroy()` returns would
/// cause undefined behaviour if it then tries to make any JNI calls or tries
/// to detach itself.
///
/// Normally `jni-rs` will automatically detach threads from the `JavaVM` by storing
/// a guard in thread-local-storage that will detach on `Drop` but this will cause
/// undefined behaviour if `JavaVM::destroy()` has been called.
///
/// Calling this will clear the thread-local-storage guard and detach the thread
/// early to avoid any attempt to automatically detach when the thread exits.
///
/// # Safety
///
/// __Any existing `JNIEnv`s and `AttachGuard`s created in the calling thread
/// will be invalidated after this method completes. It is the__ caller’s __responsibility
/// to ensure that no JNI calls are subsequently performed on these objects.__
/// Failure to do so will result in unspecified errors, possibly, the process crash.
///
/// Given some care is exercised, this method can be used to detach permanently attached
/// threads _before_ they exit (when automatic detachment occurs). However, it is
/// never appropriate to use it with the scoped attachment (`attach_current_thread`).
// This method is hidden because it is almost never needed and its use requires some
// extra care. Its status might be reconsidered if we learn of any use cases that require it.
pub unsafe fn detach_current_thread(&self)

To me this just highlights that the feature doesn't make any sense here.

The JNI concept of a daemon thread is something that's only relevant to applications that would ever call JavaVM::destroy() which in itself isn't that useful for most applications.

It's also notable that JNI doesn't technically support destroying a VM (not in the sense that you can then create another one to replace it) and so calling JavaVM::destroy() really just puts the VM into a poorly specified limbo state until the process exits.

The main reason applications would want to call ``JavaVM::destroy()` is for the side effect that it waits until all (non-daemon) threads have exited.

Really though, the daemon thread attachments are only a convenience with a very dangerous trade off.

Applications either need to pay a price with being extra careful to somehow ensure daemon threads can't access the JVM after its destroyed or else they need some mechanism to trigger those threads to exit when the application is finished.

Additionally, considering that we have other open questions / issues relating to how this crate supports thread attachments (e.g. #441) I think it'd be best to try and simplify things and no longer expose daemon thread attachments.

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

No branches or pull requests

1 participant