Skip to content

fix(jni): detach native threads and preserve callbacks - #51

Open
AlexProgrammerDE wants to merge 3 commits into
pschichtel:stablefrom
enderdash-com:agent/fix-jni-thread-lifecycle-stable
Open

fix(jni): detach native threads and preserve callbacks#51
AlexProgrammerDE wants to merge 3 commits into
pschichtel:stablefrom
enderdash-com:agent/fix-jni-thread-lifecycle-stable

Conversation

@AlexProgrammerDE

Copy link
Copy Markdown

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:

Copilot AI lite review requested due to automatic review settings August 9, 2026 19:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@AlexProgrammerDE
AlexProgrammerDE force-pushed the agent/fix-jni-thread-lifecycle-stable branch from 4fc3f71 to 826d08b Compare August 9, 2026 20:21
@pschichtel

Copy link
Copy Markdown
Owner

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

@AlexProgrammerDE

Copy link
Copy Markdown
Author
  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

Copy link
Copy Markdown
Author

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

@AlexProgrammerDE

Copy link
Copy Markdown
Author

Hi, any update on this?

@pschichtel

Copy link
Copy Markdown
Owner

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.

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.

3 participants