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

Delete Dart Functions associated with ObjC Blocks #204

Closed
liamappelbe opened this issue Jul 27, 2022 · 8 comments · Fixed by #1100
Closed

Delete Dart Functions associated with ObjC Blocks #204

liamappelbe opened this issue Jul 27, 2022 · 8 comments · Fixed by #1100

Comments

@liamappelbe
Copy link
Contributor

liamappelbe commented Jul 27, 2022

This is not feasible to do at the moment, because NativeFinalizers can't call functions created by Pointer.fromFunction. See #233 for details.

This will be possible once dart-lang/sdk#47778 is fixed

UPDATE (27/10/23): The fix we're going with is to ship a small piece of native code in a support library that will notify the owner isolate when the block is deleted. This is blocked on the native assets feature (see dart-lang/sdk#50565 and flutter/flutter#129757).

@liamappelbe
Copy link
Contributor Author

Actually, this could probably also be solved by async callbacks. The Block struct's dispose field doesn't have the restriction mentioned above (that NativeFinalizers can't call functions created by Pointer.fromFunction), because it isn't directly called by the NativeFinalizer (it's called automatically when the Block is cleaned up). We still have the issue that the dispose function can be called on a random thread, but that's what async callbacks are for. This works as long as async callbacks can fail gracefully in the case where the VM is in the middle of shutting down (it's fine to leak the function in that case).

@mkustermann
Copy link
Member

This is not feasible to do at the moment, because NativeFinalizers can't call functions created by Pointer.fromFunction. See #233 for details.

Actually, this could probably also be solved by async callbacks.

It is feasible by generating C code as outlined in #233. The asynchronous callbacks will inevitably also have to be based on generated C code, for very similar reasons as here (namely that there's objects in the ObjC block that need to be pulled out and used before executing dart code)

One could avoid generating C code by using isolate independent code instead (which is C written in Dart DSL), but current plan is to not have isolate independent code and instead make it seamless for packages to have accompanying native code.

One could also avoid generating C code by baking ObjC specific knowledge into the Dart compiler, but that doesn't seem like a good idea (the Dart compiler would need to rely on assumptions made by a unrelated code generator)

@liamappelbe
Copy link
Contributor Author

The asynchronous callbacks will inevitably also have to be based on generated C code, for very similar reasons as here (namely that there's objects in the ObjC block that need to be pulled out and used before executing dart code)

Not quite sure what you mean. If I write the dispose function in Dart, and stick it in the dispose field of the Block, then it should work, right? The dispose function could be called from an arbitrary thread (or during isolate shutdown), but that's solved by making it an async callback.

@mkustermann
Copy link
Member

Not quite sure what you mean. If I write the dispose function in Dart, and stick it in the dispose field of the Block, then it should work, right? The dispose function could be called from an arbitrary thread (or during isolate shutdown), but that's solved by making it an async callback.

If the dispose is called on an arbitrary thread, the dispose method invoked gets a block (whose contents is specific to objc binding generator). Before we can execute any dart code, we'll have to enter an isolate, so the code being invoked must pull out information from the block in the right place to enter an isolate (and that code cannot be written in Dart)

@liamappelbe
Copy link
Contributor Author

Before we can execute any dart code, we'll have to enter an isolate, so the code being invoked must pull out information from the block in the right place to enter an isolate

I thought the whole point of async callbacks was to do this automatically. Once we have async callbacks, the VM will have a trampoline that makes sure to run the callback in an isolate (details TBD), rather than just crashing.

@liamappelbe
Copy link
Contributor Author

liamappelbe commented Sep 14, 2023

Status update: We still need to clean up the Dart metadata associated with ObjC blocks when the block's ref count hits 0. The ref count can be decremented by Dart or ObjC code. In the case of ObjC that can happen from any thread, so I want to put a NativeCallable.listener in the block struct's dispose_helper field. From the Dart side, the ref count is decremented when the Dart wrapper object (ObjCBlock) is collected by GC, triggering a NativeFinalizer that calls the _Block_release (a C function from the ObjC runtime). But currently there's a deadlock bug with this approach, because we're trying to run the NativeCallable.listener during a GC safepoint.

image

Next steps:

  • One approach would be to fix the GC deadlock by deferring NativeFinalizers until after the GC safepoint.
  • Another approach would be to use a C function for the dispose_helper instead of a NativeCallable.listener, which would still send a message to the target isolate, but wouldn't run into the deadlock. This C function would be generic enough that we could put it in a shared library like package:ffi, so we can still avoid having ffigen generate C code. But this approach is blocked on the native-assets feature that will make it ergonomic to ship native code in a Dart package.

@dcharkes
Copy link
Collaborator

One approach would be to fix the GC deadlock by deferring NativeFinalizers until after the GC safepoint.

When we designed the native finalizers we explicitly banned calling back into Dart code, as it would break synchronous Dart semantics: running some other Dart code than the mutator thread without having a yield. Now the helper isolate is kind of special, because we know it wont break Dart semantics. However, it does complicate the GC implementation, so we need to think carefully if we would break something. E.g. we can't use messages via ports, because we guarantee running native finalizers on isolate group shutdown. So it would need to be some kind of data structure in the isolate group instead.

  • Another approach would be to use a C function for the dispose_helper instead of a NativeCallable.listener, which would still send a message to the target isolate, but wouldn't run into the deadlock. This C function would be generic enough that we could put it in a shared library like package:ffi, so we can still avoid having ffigen generate C code. But this approach is blocked on the native-assets feature that will make it ergonomic to ship native code in a Dart package.

As discussed offline, this needs dart-lang/sdk#50565 and flutter/flutter#129757. (Working hard on getting this done.)

And when that's available, we can put the code in package:objective_c (or package:objc).

Yeah, I'm planning to add an ObjC library soon, for the shared classes. Putting that native code in a shared library definitely decreases my aversion to native code. If native-assets can make it so that users don't even realize that package:objc contains native code, then I don't have a problem with it.

Posted by @liamappelbe in chat.

@liamappelbe
Copy link
Contributor Author

Now that I've mulled over the various options, I think the best approach would be a native function in package:objective_c. I think we could fix the GC deadlock, but messing with that sort of stuff is a little error prone, and the native function approach is just so much simpler. There's not much point wading into GC+threading problems if I don't have to. I'm also not in any rush to fix this minor leak, so I'll just wait for native assets to be ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants