-
Notifications
You must be signed in to change notification settings - Fork 51
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
Implementing destructors for Kotlin callbacks #682
Conversation
@@ -67,7 +68,11 @@ internal class DiplomatCallback_MyNativeStruct_test_multi_arg_callback_diplomatC | |||
} | |||
} | |||
@JvmField | |||
internal var destructor: Pointer = Pointer(0L); | |||
internal var destructor: Callback = object : Callback { |
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.
Do you need to produce an extra Callback
layer here? Is there any reason we can't just assign destroy_rust_jvm_cookie
to this directly? Having two layers of indirection seems weird unless this is actually mandatory.
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.
I've tried this but it doesn't seem to work -- as I understand this, i'ts because the Callback
is a JNA construct, and also you don't seem to be able to assign functions directly to variables in Kotlin 😢
But I'm not a Kotlin expert, maybe I'm missing something
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.
If we can figure out how to return a function pointer to the Rust function, from Rust, then we can probably have the destructor be a Pointer
instead?
9cc2f81
to
5686be7
Compare
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.
This seems correct to me, I'd like @maurer to also approve this
feature_tests/kotlin/somelib/src/main/kotlin/dev/diplomattest/somelib/CallbackWrapper.kt
Show resolved
Hide resolved
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.
This makes sense to me. I'm not the biggest fan of the extra layers of indirection and the inversion of GC vs owned environment maintenance, but it's simple, it avoids extra build system work, it doesn't modify the ABI, and it works, so seems fine.
(Above assumes you fix the link error showing up in ci, but since I've seen this working on your machine I assume that won't be hard.)
Probably worth filing a followup issue about this to try and improve the situation? |
Adding support for proper memory management of callbacks passed to Rust from Kotlin -- now, they are stopped from being freed in Rust by a JNI
GlobalRef
to the callback wrapper object on the Kotlin side. TheGlobalRef
is removed in a finalizer on the Kotlin side.This is addressing issue #648