fix(jni): detach native threads and preserve callbacks

build.gradle.kts

@@ -184,6 +184,7 @@ val dockcrossOutputDir: Directory = project.layout.buildDirectory.get().dir("doc
 val nativeForHostOutputDir: Directory = dockcrossOutputDir.dir("host")
 val compileNativeForHost by tasks.registering(DockcrossRunTask::class) {
     baseConfigure(nativeForHostOutputDir, BuildTarget(image = null, family = "host", classifier = "host"))
+    extraEnv.put("BUILD_JNI_TESTS", "ON")
     unsafeWritableMountSource = true
     runner(NonContainerRunner)
 }

conventions/src/main/kotlin/tel.schich.libdatachannel.convention.common.gradle.kts

@@ -44,6 +44,7 @@ dependencies {
     implementation(libs.slf4j)
 
     testImplementation(libs.junitJupiter)
+    testRuntimeOnly(libs.junitPlatformLauncher)
     testImplementation(libs.logbackClassic)
 }
 

gradle/libs.versions.toml

@@ -12,6 +12,7 @@ jniAccessGenerator = { module = "tel.schich:jni-access-generator", version.ref =
 jdtAnnotations = { module = "org.eclipse.jdt:org.eclipse.jdt.annotation", version.ref = "jdtAnnotations" }
 slf4j = { module = "org.slf4j:slf4j-api", version.ref = "slf4j" }
 logbackClassic = { module = "ch.qos.logback:logback-classic", version.ref = "logback" }
+junitPlatformLauncher = { module = "org.junit.platform:junit-platform-launcher", version.ref = "junitJupiter" }
 junitJupiter = { module = "org.junit.jupiter:junit-jupiter-engine", version.ref = "junitJupiter" }
 
 [plugins]

jni/CMakeLists.txt

@@ -5,6 +5,7 @@ set(CMAKE_C_STANDARD 11)
 set(CMAKE_CXX_STANDARD 11)
 
 option(PROJECT_VERSION "The version of the project" "unspecified")
+option(BUILD_JNI_TESTS "Build JNI test support" OFF)
 
 set(NO_WEBSOCKET ON CACHE BOOL "configure libdatachannel build")
 set(NO_MEDIA ON CACHE BOOL "configure libdatachannel build")
@@ -54,6 +55,19 @@ add_library(datachannel-java SHARED
         src/util.c
         src/native_channel.c
         src/native_peer.c
+        src/peer_connection_lifecycle.c
         src/native_track.c
         src/callback.c)
+if(BUILD_JNI_TESTS)
+    target_sources(datachannel-java PRIVATE test/thread_lifecycle.c)
+
+    enable_testing()
+    find_package(Threads REQUIRED)
+    add_executable(datachannel-java-native-tests
+            test/native_lifecycle_test.c
+            src/init.c
+            src/peer_connection_lifecycle.c)
+    target_link_libraries(datachannel-java-native-tests PRIVATE Threads::Threads)
+    add_test(NAME datachannel-java-native-tests COMMAND datachannel-java-native-tests)
+endif()
 target_link_libraries(datachannel-java PRIVATE datachannel-static)

jni/build.sh

@@ -27,6 +27,7 @@ cmake_options=(
   "-DCMAKE_PROJECT_TOP_LEVEL_INCLUDES=${MOUNT_SOURCE}/jni/cmake-conan/conan_provider.cmake"
   "-DPROJECT_VERSION=${PROJECT_VERSION}"
   "-DCMAKE_BUILD_TYPE=${PROJECT_BUILD_TYPE}"
+  "-DBUILD_JNI_TESTS=${BUILD_JNI_TESTS:-OFF}"
 )
 
 if [ "$TARGET_FAMILY" = 'android' ]
@@ -46,4 +47,8 @@ then
 fi
 
 cmake "$RELATIVE_PROJECT_PATH" "${cmake_options[@]}"
-make -j"${JOBS:-1}"
+make -j"${JOBS:-1}"
+if [ "${BUILD_JNI_TESTS:-OFF}" = 'ON' ]
+then
+  ctest --output-on-failure
+fi

jni/src/init.c

@@ -3,40 +3,66 @@
 #include <jni.h>
 #include <pthread.h>
 #include <rtc/rtc.h>
+#include <stdbool.h>
 
 #define JNI_VERSION JNI_VERSION_1_6
 
-static JavaVM* global_JVM;
+static pthread_mutex_t lifecycle_mutex = PTHREAD_MUTEX_INITIALIZER;
+static JavaVM* global_jvm;
 static pthread_key_t thread_key;
+static bool thread_key_initialized;
+static bool jvm_unloading = true;
 
-void detach_thread() {
-    JavaVM* jvm = pthread_getspecific(thread_key);
+static void stop_jvm_access(void) {
+    pthread_mutex_lock(&lifecycle_mutex);
+    jvm_unloading = true;
+    global_jvm = NULL;
+    pthread_mutex_unlock(&lifecycle_mutex);
+}
+
+static void delete_thread_key(void) {
+    pthread_mutex_lock(&lifecycle_mutex);
+    if (thread_key_initialized) {
+        pthread_key_delete(thread_key);
+        thread_key_initialized = false;
+    }
+    pthread_mutex_unlock(&lifecycle_mutex);
+}
+
+static void detach_thread(void* value) {
+    JavaVM* jvm = value;
     if (jvm != NULL) {
         (*jvm)->DetachCurrentThread(jvm);
     }
 }
 
-JNIEnv* get_jni_env_from_jvm(JavaVM* jvm) {
-    JNIEnv* env;
+JNIEnv* get_jni_env(void) {
+    pthread_mutex_lock(&lifecycle_mutex);
+    if (global_jvm == NULL || jvm_unloading || !thread_key_initialized) {
+        pthread_mutex_unlock(&lifecycle_mutex);
+        return NULL;
+    }
+
+    JNIEnv* env = NULL;
+    JavaVM* jvm = global_jvm;
     jint result = (*jvm)->GetEnv(jvm, (void**) &env, JNI_VERSION);
     if (result == JNI_EDETACHED) {
         result = (*jvm)->AttachCurrentThreadAsDaemon(jvm, (void**) &env, NULL);
         if (result == JNI_OK) {
-            pthread_setspecific(thread_key, jvm);
+            if (pthread_setspecific(thread_key, jvm) != 0) {
+                (*jvm)->DetachCurrentThread(jvm);
+                pthread_mutex_unlock(&lifecycle_mutex);
+                return NULL;
+            }
         }
     }
-    if (result != JNI_OK) {
+    if (result != JNI_OK || env == NULL) {
+        pthread_mutex_unlock(&lifecycle_mutex);
         return NULL;
     }
-    return env;
-}
 
-JNIEnv* get_jni_env() {
-    // make sure it's initialized
-    if (global_JVM == NULL) {
-        return NULL;
-    }
-    return get_jni_env_from_jvm(global_JVM);
+    pthread_mutex_unlock(&lifecycle_mutex);
+    return env;
 }
 
 void logger_callback(rtcLogLevel level, const char* message) {
@@ -50,18 +76,52 @@ void logger_callback(rtcLogLevel level, const char* message) {
 }
 
 JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM* jvm, void* reserved) {
-    pthread_key_create(&thread_key, detach_thread);
-    global_JVM = jvm;
-    JNIEnv* env = get_jni_env_from_jvm(jvm);
+    if (jvm == NULL || pthread_key_create(&thread_key, detach_thread) != 0) {
+        return JNI_ERR;
+    }
+    pthread_mutex_lock(&lifecycle_mutex);
+    thread_key_initialized = true;
+    pthread_mutex_unlock(&lifecycle_mutex);
+
+    JNIEnv* env = NULL;
+    if ((*jvm)->GetEnv(jvm, (void**) &env, JNI_VERSION) != JNI_OK || env == NULL) {
+        delete_thread_key();
+        return JNI_ERR;
+    }
+
     module_OnLoad(env);
+    if ((*env)->ExceptionCheck(env)) {
+        module_OnUnload(env);
+        delete_thread_key();
+        return JNI_ERR;
+    }
+
+    pthread_mutex_lock(&lifecycle_mutex);
+    global_jvm = jvm;
+    jvm_unloading = false;
+    pthread_mutex_unlock(&lifecycle_mutex);
+
     rtcInitLogger(RTC_LOG_VERBOSE, &logger_callback);
     rtcPreload();
+    if ((*env)->ExceptionCheck(env)) {
+        stop_jvm_access();
+        rtcInitLogger(RTC_LOG_NONE, NULL);
+        rtcCleanup();
+        module_OnUnload(env);
+        delete_thread_key();
+        return JNI_ERR;
+    }
     return JNI_VERSION;
 }
 
 JNIEXPORT void JNICALL JNI_OnUnload(JavaVM* jvm, void* reserved) {
+    stop_jvm_access();
+    rtcInitLogger(RTC_LOG_NONE, NULL);
     rtcCleanup();
-    JNIEnv* env = get_jni_env();
-    module_OnUnload(env);
-    global_JVM = NULL;
-}
+
+    JNIEnv* env = NULL;
+    if (jvm != NULL && (*jvm)->GetEnv(jvm, (void**) &env, JNI_VERSION) == JNI_OK && env != NULL) {
+        module_OnUnload(env);
+    }
+    delete_thread_key();
+}

jni/src/native_peer.c

@@ -1,4 +1,5 @@
 #include "callback.h"
+#include "peer_connection_lifecycle.h"
 #include "util.h"
 #include <jni-c-to-java.h>
 #include <jni-java-to-c.h>
@@ -138,12 +139,7 @@ Java_tel_schich_libdatachannel_LibDataChannelNative_rtcClosePeerConnection(JNIEn
 JNIEXPORT jint JNICALL
 Java_tel_schich_libdatachannel_LibDataChannelNative_rtcDeletePeerConnection(JNIEnv* env, jclass clazz,
                                                                             jint peerHandle) {
-    struct jvm_callback* callback = rtcGetUserPointer(peerHandle);
-    if (callback != NULL) {
-        free_callback(env, callback);
-    }
-
-    return rtcDeletePeerConnection(peerHandle);
+    return delete_peer_connection(env, peerHandle);
 }
 
 
@@ -270,4 +266,4 @@ JNIEXPORT jint JNICALL Java_tel_schich_libdatachannel_LibDataChannelNative_setup
     rtcSetUserPointer(peerHandle, jvm_callback);
 
     return RTC_ERR_SUCCESS;
-}
+}

jni/src/peer_connection_lifecycle.c

@@ -0,0 +1,14 @@
+#include "peer_connection_lifecycle.h"
+
+#include "callback.h"
+#include <rtc/rtc.h>
+
+jint delete_peer_connection(JNIEnv* env, jint peer_handle) {
+    struct jvm_callback* callback = rtcGetUserPointer(peer_handle);
+    jint result = rtcDeletePeerConnection(peer_handle);
+    if (result == RTC_ERR_SUCCESS && callback != NULL) {
+        free_callback(env, callback);
+    }
+
+    return result;
+}

jni/src/peer_connection_lifecycle.h

@@ -0,0 +1,8 @@
+#ifndef LIBDATACHANNEL_JNI_PEER_CONNECTION_LIFECYCLE_H
+#define LIBDATACHANNEL_JNI_PEER_CONNECTION_LIFECYCLE_H
+
+#include <jni.h>
+
+jint delete_peer_connection(JNIEnv* env, jint peer_handle);
+
+#endif//LIBDATACHANNEL_JNI_PEER_CONNECTION_LIFECYCLE_H