fix(jni): detach native threads and preserve callbacks

[AlexProgrammerDE]

Summary

• detach JNI-attached native threads with the value passed to the POSIX TLS destructor
• serialize JVM access, native-thread attachment, and unload transitions in the stable C bindings
• reject failed JVM and pthread initialization instead of continuing with partial state
• keep the peer callback allocation and JNI global reference alive until rtcDeletePeerConnection drains scheduled callbacks
• add a host-only JNI regression test that attaches a pthread, lets it terminate, and verifies HotSpot marks its Java Thread as terminated
• add the JUnit Platform launcher required by Gradle 9.3.1 to execute the new stable-branch test

Root cause

POSIX clears a thread-specific key before calling its destructor and passes the previous value as the destructor argument. The current destructor ignores that argument and calls pthread_getspecific(thread_key), which therefore returns NULL. DetachCurrentThread is skipped and the terminated native thread remains registered with the JVM.

The surrounding JVM lifecycle also permits unsafe partial states. The JVM pointer and unload state were accessed without common synchronization, pthread key creation and thread-specific registration failures were ignored, and JNI_OnUnload relied on the shared environment accessor while tearing down the same state.

The peer deletion wrapper has a separate lifetime inversion. It frees the callback allocation and its JNI global reference before calling rtcDeletePeerConnection, even though that operation is what blocks until scheduled callbacks return. A callback racing with deletion can consequently dereference the freed user pointer.

This replaces #50 with the same fixes ported to the production-oriented stable branch and its C JNI implementation.

Lifecycle hardening

JVM lifecycle state is now protected by one mutex so unloading cannot delete the pthread key while a callback thread is between attachment and TLS registration. A failed pthread_setspecific immediately detaches the thread. Load failures clean up the pthread key and return JNI_ERR; initialization exceptions also release cached global references and preloaded RTC state.

Unload first prevents new JVM access, disables Java logging callbacks, drains libdatachannel, obtains the unloading thread's existing JNIEnv* directly from the supplied JavaVM*, releases cached global references when an environment is available, and finally deletes the pthread key.

Impact

Terminated callback threads no longer leave stale attached-thread records in HotSpot. Load and unload cannot publish or retain partially initialized JNI state, and peer teardown retains its callback state until libdatachannel guarantees that no pending callback can use it.

Validation

• Ran ./gradlew check --no-configuration-cache successfully on Linux with JDK 25.
• Confirmed the native C library and test helper compile with warnings treated as errors.
• Confirmed NativeThreadLifecycleTest runs and passes.

References:

• POSIX pthread_key_create destructor semantics
• JNI Invocation API thread detachment requirement
• JNI native-library lifecycle hooks
• OpenJDK regression test for a native thread that terminates without detaching

[Copilot]

Pull request overview

This PR hardens the JNI lifecycle and callback teardown behavior for the stable C bindings of libdatachannel-java, addressing native-thread detachment correctness, unload safety, and callback lifetime ordering. It also adds a host-only JNI regression test to validate that native pthread termination no longer leaves stale attached-thread records in HotSpot.

Changes:

• Serialize JVM access / native-thread attachment / unload using a single mutex and correct POSIX TLS destructor usage for detaching threads.
• Make initialization/unload paths fail-fast and cleanup on partial state (pthread key creation, GetEnv, module init/preload exceptions).
• Fix peer deletion callback lifetime ordering and add a host-only regression test + Gradle runtime dependency for the JUnit Platform launcher.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/test/java/tel/schich/libdatachannel/NativeThreadLifecycleTest.java	Adds a JUnit regression test asserting terminated native threads are detached and observed as TERMINATED.
jni/test/thread_lifecycle.c	Implements the JNI native helper that spawns/joins a pthread and returns its Thread object for assertions.
jni/src/native_peer.c	Ensures callback/global-ref cleanup occurs only after rtcDeletePeerConnection completes successfully.
jni/src/init.c	Adds mutex-guarded JVM lifecycle state, correct TLS destructor handling, and stricter init/unload cleanup paths.
jni/CMakeLists.txt	Adds BUILD_JNI_TESTS option to include JNI test sources only for host builds.
jni/build.sh	Wires BUILD_JNI_TESTS through to CMake.
gradle/libs.versions.toml	Adds the JUnit Platform launcher dependency entry.
conventions/src/main/kotlin/tel.schich.libdatachannel.convention.common.gradle.kts	Adds testRuntimeOnly for the JUnit Platform launcher so tests run under newer Gradle.
build.gradle.kts	Enables BUILD_JNI_TESTS=ON for host native builds used by the JVM test suite.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[pschichtel]

Can you elaborate a bit on what concrete issues this fixes?

[AlexProgrammerDE]

1. Fixes that threads can be left in a bad native detched state
2. Fixes that we free the callback before we have properly used it
3. Fixes jvm start/stop lifecycle ordering issues that could cause a race condition in very rare cases
4. Properly call finalizers (some calls were ordered in a way that the finalizers would always be called on a null pointer and not properly finalize the object)

[AlexProgrammerDE]

I also added a unit test for properly start/stop and thread state.

[AlexProgrammerDE]

Hi, any update on this?

[pschichtel]

This is not a small change, so I have to find some proper time to get into this. I'm still fairly uncertain about the changes around the lifecycle, so I'll have to do some research on that for my own understanding if there really is an issue.

Can you introduce tests for all the issues you had, ideally without relying on JNI?

For example (2) and (4) feel like they should be reproducible in tests. For (1) and (3) I'm not sure how these issues would present themselves.

[AlexProgrammerDE]

I've introduced more tests that now cover all problems that this PR fixes. Here's an overview: (written by AI, verified locally)

Production change removed or reverted	Test that fails	Observed failure
Reject NULL in JNI_OnLoad	rejects missing JVM	Native test process crashes with exit code 139 instead of returning JNI_ERR
Check the result of JavaVM.GetEnv during JNI_OnLoad	cleans up failed GetEnv	Native test process crashes with exit code 139 instead of returning JNI_ERR
Call module_OnUnload, delete the pthread key, and return JNI_ERR when module_OnLoad leaves a pending exception	cleans up failed module load	Expected module cleanup does not occur
Revoke JVM access, disable logging, call rtcCleanup, unload the module, delete the pthread key, and return JNI_ERR when rtcPreload leaves a pending exception	cleans up failed RTC preload	JNI_OnLoad incorrectly succeeds and cleanup does not occur
Use the value passed to the pthread TLS destructor instead of calling pthread_getspecific	detaches attached native threads	Attached native threads terminate without calling DetachCurrentThread
Disable the libdatachannel logger before calling rtcCleanup during JNI_OnUnload	unloads in safe order	Recorded unload operations occur in the wrong order
Revoke JNI access before native cleanup during JNI_OnUnload	unloads in safe order	JNI remains available during teardown and the expected lifecycle order is violated
Call rtcDeletePeerConnection before freeing its callback state	frees callback after successful peer deletion	Callback state is freed before rtcDeletePeerConnection drains scheduled callbacks
Free callback state only when rtcDeletePeerConnection succeeds	retains callback when peer deletion fails	Callback state is freed even though the peer still exists after failed deletion
Guard callback cleanup when the peer has no callback state	deletes peer without callback state	Cleanup is incorrectly attempted for a missing callback

[AlexProgrammerDE]

Hi, any update on this?

[pschichtel]

I will have a look into this on the weekend.

[pschichtel]

When would either of this ever happen? Given that JNI_OnLoad would only ever be called once when loading the library.

[AlexProgrammerDE]

pthread_key_create can fail on its first invocation with EAGAIN or ENOMEM. That's why we validate it here.
The jvm NULL check is theoretically unnecessary, but it's just defensive validation. I can remove it if you want.

[pschichtel]

well true, but is that likely to show up here? Have you seen these?

[pschichtel]

The locking around the "lifecycle transitions" seems rather convoluted for what is a single problematic user: the logging. All other it's the only caller of the get_jni_env(void) function and I'd prefer if that wouldn't exist. The C++ version completely avoids this mess, it's an unfortunate design in the C API. A simple attempt to change it upstream wasn't accepted a while back.

Are there other reasons for the locking? Also did this actually cause issues? Because if this is a theoretical issue I'd rather not fix it in the C version.

[pschichtel]

Leaking the thread key seems like a valid issue, but on the other hand: the Java side does not provide a way to unload the library anyway. Failing to load the library is the only case that could actually leak the key, but unless you are doing that a lot in the same process I'm not convinced this is worth fixing. Once again the C++ rewrite doesn't have this issue as it doesn't rely on thread keys. Memory safety/management is actually a focus in the rewrite.

[pschichtel]

From what I can tell assuming my previous statements are valid, this leaves me with 2 changes that I think are worth pulling:

the init.c changes from your first commit and the native_peer.c from the third.

Would you be able to provide tests written in Java that fail without these changes?