Thread detach hook for Java JNI on Android #250
Conversation
dispatch/queue.h
Outdated
| * "detached" before the thread exits or the application will crash. | ||
| */ | ||
| DISPATCH_EXPORT | ||
| void (*dispatch_thread_detach_handler)(); |
There was a problem hiding this comment.
- this should go to queue_internal.h
- this should be
(void) - this should only be enabled on android
There was a problem hiding this comment.
Changes made. queue_interal.h doesn’t seem to be visible from Swift so I have left it where it is and prefixed the symbol with an “_" for now.
src/queue.c
Outdated
| @@ -861,6 +861,8 @@ gettid(void) | |||
| if ((f) && tsd->k) ((void(*)(void*))(f))(tsd->k); \ | |||
| } while (0) | |||
|
|
|||
| void (*dispatch_thread_detach_handler)(); | |||
There was a problem hiding this comment.
Should be on android only
dispatch/queue.h
Outdated
| * "detached" before the thread exits or the application will crash. | ||
| */ | ||
| DISPATCH_EXPORT | ||
| void (*_dispatch_thread_detach_callback)(void); |
There was a problem hiding this comment.
for the second time this is NOT appropriate for an API header, this belongs to an internal one.
if the swift overlay is not built seeing the internal headers, then either try to fix that, or extern the prototype here again. it is an internal implementation detail between the overlay and libdispatch that should never be exposed anywhere.
this HAS to move to queue_internal.h, or maybe "private/private.h" this could be appropriate as well, and would likely be seen by the overlay.
There was a problem hiding this comment.
There seems to be no way to expose the internal dispatch headers to the Swift side of things. There is a DispatchPrivate module but it won’t import during the build. So, I’ve used a @_silgen_name to pick up a setter function which isn’t ideal but at least it no longer affects the public api.
src/queue.c
Outdated
| @@ -885,6 +889,10 @@ _libdispatch_tsd_cleanup(void *ctx) | |||
| _tsd_call_cleanup(dispatch_voucher_key, _voucher_thread_cleanup); | |||
| _tsd_call_cleanup(dispatch_deferred_items_key, | |||
| _dispatch_deferred_items_cleanup); | |||
| #ifdef __ANDROID__ | |||
| if (_dispatch_thread_detach_callback) | |||
There was a problem hiding this comment.
If it was like this before, sorry to have missed it, please add brackets.
| @@ -861,6 +861,14 @@ gettid(void) | |||
| if ((f) && tsd->k) ((void(*)(void*))(f))(tsd->k); \ | |||
| } while (0) | |||
|
|
|||
| #ifdef __ANDROID__ | |||
| static void (*_dispatch_thread_detach_callback)(void); | |||
There was a problem hiding this comment.
if it's static then the overlay which is a dynamic library can't pick it up.
@das suggested to me the right place is to export the symbol is from private/queue_private.h or private/private.h and also to use silgen like you did.
There was a problem hiding this comment.
Global symbol is now a function _dispatch_set_detach_callback_ptr as it doesn’t seem possible to express a global var in a silgen. I’ve moved it's declaration into queue_private.h.
src/queue.c
Outdated
| static void (*_dispatch_thread_detach_callback)(void); | ||
|
|
||
| void | ||
| *(*_dispatch_thread_detach_callback_ptr(void))(void) { |
There was a problem hiding this comment.
this is hard to read, also exposing a pointer to a function pointer is really weird. do you really need the getter? it feels like you only want to install it, I would have gone for:
void
_dispatch_install_thread_detach_callback(dispatch_function_t cb)
{
if (os_atomic_xchg(&_dispatch_thread_detach_callback, cb, relaxed)) {
DISPATCH_CLIENT_CRASH(0, "Installing a thread detach callback twice");
}
}
src/swift/Queue.swift
Outdated
| @_silgen_name("_dispatch_thread_detach_callback_ptr") | ||
| private static func _dispatch_thread_detach_callback_ptr() -> UnsafeMutablePointer<(@convention(c) () -> Void)?> | ||
|
|
||
| public static var threadDetachCallback: (@convention(c) () -> Void)? { |
There was a problem hiding this comment.
I think only a one-way setter should be exposed. having a getter makes the interface un-necessarily ugly.
There was a problem hiding this comment.
Thanks for the potiners. It helps me zero in on something you’re happy with. Changed pushed.
|
@MadCoder Any changes you’d like on the latest version? |
|
Hi @MadCoder, any chance we could arrive at something that is agreeable to you that we could merge? I’d liek to tie the api down so I can release a Swift/Andoird precompiled toolchain. While it clutters the dispatch api a tad it is pretty much a requirement for apps mixing Java and Swift that use dispatch queues. |
|
Sorry I had forgotten about this pull request, the latest changes looks good, please squash it in a single commit so that I can merge. |
|
Can’t you do that with a button in github as you merge for me? I can look at it but my git skills are very limited! |
Thread detach hook for Java JNI on Android II Thread detach hook for Java JNI on Android III Renamed for consistency Revert public API change Move to queue_private.h Implemented getter Single entry point Setter only
|
Bah, tried a rebase and there are now 11 commits :( Changes are still correct. Can you squish as you merge please? |
|
Try
|
|
Thanks but I’m afraid that hasn’t worked.. conflicts etc locally now. Can you not squish as you merge? Otherwise I’ll have to start a new clone, remake the changes retest and resubmit. |
|
I’ve resubmitted this as PR #259 after remaking the changes to a clean clone of up to date repo. |
This is a callback hook required when mixing multi threaded Swift and Java on Android. When Java JNI has been used on a thread the thread needs to be detached from JNI before the thread exits to free local variables or the application will crash e.g. https://github.com/SwiftJava/java_swift/blob/master/Sources/JavaJNI.swift#L20.
Introducing this hook gives any future Java integration code a chance to do this.