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

JVM doesn't terminate when using Hybrids #25

Open
dreiss opened this issue Nov 16, 2019 · 7 comments · May be fixed by #67
Open

JVM doesn't terminate when using Hybrids #25

dreiss opened this issue Nov 16, 2019 · 7 comments · May be fixed by #67

Comments

@dreiss
Copy link
Contributor

dreiss commented Nov 16, 2019

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

jxtps commented Aug 18, 2021

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

jxtps commented Aug 18, 2021

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.

@jxtps jxtps linked a pull request Aug 18, 2021 that will close this issue
@dreiss
Copy link
Contributor Author

dreiss commented Aug 18, 2021

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

jxtps commented Aug 24, 2021

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

jxtps commented Aug 24, 2021

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

jxtps commented Dec 31, 2021

@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
Copy link

pkozelka commented Sep 7, 2022

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.

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 a pull request may close this issue.

3 participants