dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.27k stars 1.58k forks source link

[vm/ffi] `KeepAlive` marker interface #56227

Open dcharkes opened 4 months ago

dcharkes commented 4 months ago

We currently have a Finalizable interface that serves two purposes:

  1. It keeps objects alive until the end of the syntactic scope.
  2. It can have NativeFinalizers attached.

Due to (2), we disallow sending objects implementing Finalizable to other isolates. As that that will most likely be an error.

However, when attaching finalizers via the dart_api.h, we also need the behavior of (1), and we actually want to send objects to other isolates (that's why we resorted to using finalizers from the dart_api.h in the first place.)

We should introduce another marker interface that only has behavior 1. Maybe the interfaces should even be related.

Ready for bike shedding on the name:

a. KeepAlive b. IsolateGroupFinalizable

The marker interface should probably live in dart:ffi as well. Because it's mostly used when sending objects via FFI with Handles. Also it would enable relating the two interfaces, helping with documentation. But I can be convinced of other options.

Use case:

cc @HosseinYousefi @mkustermann

dcharkes commented 2 months ago

@mkustermann: name option NativeFinalizale.

mkustermann commented 2 months ago

(I misremembered how this works in today's meeting)

The current setup is:

I think the main reasons we currently disallow Finalizers to be shared across isolates is that:

So marking a class as extends Finalizable and marking it as @pragma('vm:deeply-immutable') allows sending/sharing across isolates but

Eventually (with shared multi threading) we want a finalizer API that works across isolates (just like our existing C API works across isolates). So one way would be to have

But this may require two different marker interfaces or otherwise leaves room for user bugs where objects that are isolate-local are shared.

We could go instead the other direction:

=> This would effectively align the NativeFinalizer behavior with that of the C API. => This would make it safe to allow sharing Finalizable & deeply immutable objects across isolates.

So the main question is: Do we really care about early reclamation of isolate-local resources on unexpected isolate shutdown? This is only important if isolates are short lived and native resources aren't eagerly freed up by user code (or e.g. isolates get often killed) and we cannot wait for the GC to eventually run them a bit later.

/cc @mraleph wdyt?

(**) This assumption only makes sense for synchronous code. In asynchronous code one can easily have

foo() async {
  final obj = Foo(field);
  finalizer.attach(foo, ...., () => ...);
  final field = obj.field;
  await <...>
  // ^^^^
  //   - takes long time
  //   - may cause GC
  //   - may collect `obj` (as it's not used anymore & it's not `Finalizable)
  //   - GC may enqueue finalizer,
  //   - may run finalizer
  useField(field);  // <-- use field after finalizer has run
}
dcharkes commented 2 months ago

=> This would effectively align the NativeFinalizer behavior with that of the C API.

That's not entirely true, canceling finalizers from another isolate doesn't work, from a previous discussion of ours:

  • However, detach would not work shared. Trying to detach from a non-shared static nativeFinalizer would lead to a no-op on another isolate. Because it would be a different native finalizer (that happens to have the same callback address.)

Do we really care about early reclamation of isolate-local resources on unexpected isolate shutdown?

I don't know of any such use cases. So, I like the idea of trying to make NativeFinalizer shared always instead of introducing a SharedNativeFinalizer later, from another previous discussion of ours:

I believe we could make the existing NativeFinalizer sharable (and thus make detach work from any isolate) by making it's backing store for entries (a map), concurrency safe. (We would need to do the same work for introducing a SharedNativeFinalizer later anyway.)

mkustermann commented 2 months ago

That's not entirely true, canceling finalizers from another isolate doesn't work, from a previous discussion of ours:

The code may not care about detachment at all, it may only detach on the main isolate anyway, helper isolates could ask main isolate to detach or it may (in future) use static shared NativeFinalizer = ... (in which case one gets the same semantics as the C api)

Trying to detach from a non-shared static nativeFinalizer would lead to a no-op

Wouldn't it throw a nice exception: "You cannot detach object foo from this finalizer as it was not attached" ?

I believe we could make the existing NativeFinalizer sharable (and thus make detach work from any isolate) by making it's backing store for entries (a map), concurrency safe. (We would need to do the same work for introducing a SharedNativeFinalizer later anyway.)

Making NativeFinalizer shared (i.e. make them work in upcoming shared multithreading shared isolates) isn't necessary for this.

What is necessary is committing to the semantic changes of NativeFinalizer: Make them always run (even if NativeFinalizer object is GCed), make them not run on isolate shutdown. As those two semantic changes will prevent use-after-free errors.

dcharkes commented 2 months ago

Right, if you cannot send the NativeFinalizer to another isolate, you cannot try to detach there. šŸ‘

āž• For changing NativeFinalizers to be run on isolate group shutdown, and not GCable themselves if they still have attachments.

Wouldn't it throw a nice exception: "You cannot detach object foo from this finalizer as it was not attached" ?

It does not. https://github.com/dart-lang/sdk/issues/56632