Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

Provide a method to run functions on JVM thread #398

Closed
wants to merge 2 commits into from

Conversation

jonmcclung
Copy link
Contributor

This is an attempt to provide a way for C++-created threads to call into Java without risking a memory leak, as requested in #176 . The basic idea is this:

  • you pass in a lambda
  • the current thread is attached to the JVM if it's not already
  • the lambda is executed and the result, if any, stored
  • the current thread is detached only if it was this function that attached it
  • the result, if it was stored, is returned.

There are two functions because I'm not skilled enough with lambdas to make one function that will accept any kind of lambda.

@mjmacleod
Copy link
Contributor

I've tested this and confirm it works my side.

However I don't personally like the extra code clutter to use this, nor the overhead of constantly attaching/detaching.
I've implemented the code suggestions in #372 as an alternative pull request #405 that could possibly be used instead of this though I guess there is no reason they could not both co-exist also.

std::abort();
}
f();
if (attached) g_cachedJVM->DetachCurrentThread();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make the if one line if's consistent with the rest, just have a {} for if's scope like the majority.

-    if (attached) g_cachedJVM->DetachCurrentThread();
+    if (attached) {
+        g_cachedJVM->DetachCurrentThread();
+    }

std::abort();
}
auto result = f();
if (attached) g_cachedJVM->DetachCurrentThread();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make the if one line if's consistent with the rest, just have a {} for if's scope like the majority.

-    if (attached) g_cachedJVM->DetachCurrentThread();
+    if (attached) {
+        g_cachedJVM->DetachCurrentThread();
+    }

@mjmacleod
Copy link
Contributor

Just a note that #405 has been merged now, I'm not sure if there is a good reason to have more than one way to do this or not.

@shahzadlone
Copy link
Contributor

Just a note that #405 has been merged now, I'm not sure if there is a good reason to have more than one way to do this or not.

That's fair enough. I just pointed out because I have seen some pretty tedious bugs introduced because of the missing braces after an if.

@artwyman
Copy link
Contributor

artwyman commented Apr 9, 2019

#405 provided an alternative option to this. This repo isn't going to be adding any new features so I'm putting this PR on hold.

@jonmcclung jonmcclung closed this Apr 9, 2019
@jonmcclung jonmcclung deleted the run-on-jvm-thread branch April 9, 2019 13:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants