-
Notifications
You must be signed in to change notification settings - Fork 53
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
[jni] Add a call to jvm.DetachCurrentThread when the thread detaches #1163
Conversation
PR HealthChangelog Entry ✔️Details
Changes to files need to be accounted for in their respective changelogs. Coverage ✔️Details
This check for test coverage is informational (issues shown here will not fail the PR). License Headers ✔️Details
All source files should start with a license header. Unrelated files missing license headers
Package publish validation ✔️Details
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With comments addressed: lgtm
@@ -146,6 +150,11 @@ BOOL WINAPI DllMain(HINSTANCE hinstDLL, // handle to DLL module | |||
// Return FALSE to fail DLL load. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sidenote: This DllMain
is assuming the C code in package:jni
gets compiled into it's own DLL (?). That would prevent statically linking it into a bigger library (which may have already a different DllMain
). It's a little bit of a smell I think.
If we can use C++ here (if not: why?) then a global C++ object can do things in it's constructor and destructor (e.g. initializer locks and free them, create TLS keys, free them, ...) that get run on shared library load & unload times.
(Arguably this doesn't solve the issue of detaching the thread from java at exit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use C++. We would have to wrap the functions we want to call from Dart in extern "C"
but otherwise it should not be a problem. We also generate another C file from jni.h
which wraps the API and does things like conversion to global refs and exception checking.
In general, I did not really refactor the C code we had since 2 years ago. I think it's worth it to open an issue and do this in the near future. #1169
Closes #557.