JVM doesn't terminate when using Hybrids

[dreiss]

Issue description

In any program that uses HybridData, the JVM will not terminate when main returns. Calling System.exit still terminates cleanly. This appears to be because the Hybrid Destructor thread is not marked as a daemon thread. Calling sThread.setDaemon(true); resolves the issue, but I'm not sure if it can cause issues on Android.

Code example

Apply this patch:

diff --git a/host.gradle b/host.gradle
index 39f2d55..d88b96a 100644
--- a/host.gradle
+++ b/host.gradle
@@ -15,6 +15,8 @@
  */
 
 plugins {
+    id 'java'
+    id 'application'
     id 'java-library'
     id 'com.github.dcendents.android-maven' version '2.1'
     id 'com.jfrog.bintray' version '1.8.4'
@@ -47,3 +49,7 @@ POM_PACKAGING = 'jar'
 POM_ARTIFACT_ID = 'fbjni-java-only'
 
 apply from: rootProject.file('gradle/release.gradle')
+
+application {
+    mainClassName = 'com.facebook.jni.HybridRepro'
+}
diff --git a/java/com/facebook/jni/HybridRepro.java b/java/com/facebook/jni/HybridRepro.java
new file mode 100644
index 0000000..9b37056
--- /dev/null
+++ b/java/com/facebook/jni/HybridRepro.java
@@ -0,0 +1,14 @@
+package com.facebook.jni;
+
+import com.facebook.soloader.nativeloader.NativeLoader;
+import com.facebook.soloader.nativeloader.SystemDelegate;
+
+public class HybridRepro {
+  public static void main(String[] args) {
+    NativeLoader.init(new SystemDelegate());
+    new HybridData();
+    if (args.length > 0 && args[0].equals("exit")) {
+      System.exit(0);
+    }
+  }
+}

Running as ./gradlew -b host.gradle run --args nope causes the app to run forever.
Running as ./gradlew -b host.gradle run --args exit causes the app to terminate quickly.

System Info

Linux.

[jxtps]

It's been more than 2 years and this is still an issue. From what I can tell this is a textbook daemon thread?

Can we please do at least one of the following:

1. Mark the thread as a daemon on creation for all users
2. Use a system property, e.g. fbjni.setDaemon, which if set to "true" would call sThread.setDaemon(true).

I'm happy to provide a pull request (these are all O(1) line changes) if there's interest, thanks!

[jxtps]

It appears that Android doesn't really care if a thread is marked as a daemon or not: https://stackoverflow.com/a/15323954/708381

For Android purposes, daemon threads or non-daemon threads are moot. Your app's process never exits: It either remains in the background or it gets killed at some point.

This makes me vote strongly in favor of option 1, just mark the thread as a daemon for all users.

[dreiss]

Yeah, I want to do this. I just haven't had time. It's very hard to verify that this doesn't cause some subtle but insidious problem on any version of Android.

[jxtps]

User threads block the VM from exiting. Daemon threads do not. That is their superficial difference. According to https://stackoverflow.com/a/2213443/708381 (and this matches what I've read elsewhere), daemon threads are abandoned and e.g. finally blocks are not executed when the JVM exits.

This means that any Destructor instances lingering in the sReferenceQueue, could end up not having .desctruct() called on them.

That of course sounds scary - it's presumably important to call .destruct(), right?

But if the other SO post is to be believed, then this is already the case on Android: "Your app's process never exits: It either remains in the background or it gets killed at some point".

That means it was always possible for a Destructor instance to have been created and the JVM exiting before current.destruct() is called as there's no way for the JVM to exit unless you abandon this never-exiting DestructorThread, and the Destructor instance may not even have made it into the sReferenceQueue by the time the JVM exits.

[jxtps]

If you're still concerned about Android (and I don't blame you if you are), then how about:

if (!"The Android Project".equals(System.getProperty("java.vendor"))) sThread.setDaemon(true); 

Based on https://stackoverflow.com/questions/4519556/how-to-determine-if-my-app-is-running-on-android and https://developer.android.com/reference/java/lang/System#getProperties()

[jxtps]

@dreiss can we please get this resolved? I'm happy to contribute whichever of the lines below you'll accept:

if (!"The Android Project".equals(System.getProperty("java.vendor"))) sThread.setDaemon(true); 
if ("true".equals(System.getProperty("com.facebook.fbjni.setDaemon"))) sThread.setDaemon(true); 
if ("true".equals(System.getProperty("fbjni.setDaemon"))) sThread.setDaemon(true); 
sThread.setDaemon(true);  // <-- already ready & waiting in pull request #67 

If in doubt I recommend:

if ("true".equals(System.getProperty("fbjni.setDaemon"))) sThread.setDaemon(true); 

That makes it 100% opt-in, and calling code can easily put a System.setProperty("fbjni.setDaemon", "true") prior to accessing fbjni.

This is blocking normal dev operation, and it blocks normal Java process exit in production as well.

[pkozelka]

It would help me to resolve this issue, whichever way - calling System.exit() is not an option in some of my company projects.
The approach used in #67 looks good to us.