Not safe to call Dart_DeleteWeakPersistentHandle_DL during finalisation callback

[nielsenko]

According to the documentation on Dart_NewWeakPersistentHandle whenever the finalizer callback is called, then:

...
This handle has the lifetime of the current isolate. The handle can also be
explicitly deallocated by calling Dart_DeleteWeakPersistentHandle.

If the object becomes unreachable the callback is invoked with the peer as
argument. The callback can be executed on any thread, will have a current
isolate group, but will not have a current isolate. The callback can only
call Dart_DeletePersistentHandle or Dart_DeleteWeakPersistentHandle. This
gives the embedder the ability to cleanup data associated with the object.
...

but in my tests this is not the case, when the finalization is done during shutdown of the isolate. The situations is complicated by the fact that Dart_CurrentIsolateGroup is not exposed in the _DL API.

Here is an example stack trace:

./../runtime/vm/dart_api_impl.cc: 1169: error: Dart_DeleteWeakPersistentHandle expects there to be a current isolate group. Did you forget to call Dart_CreateIsolateGroup or Dart_EnterIsolate?
version=2.16.0-134.5.beta (beta) (Mon Jan 24 11:02:18 2022 +0100) on "macos_x64"
pid=10943, thread=3331, isolate_group=(nil)(0x0), isolate=(nil)(0x0)
isolate_instructions=0, vm_instructions=10752c880
  pc 0x0000000107791e74 fp 0x000070000fccec90 dart::Profiler::DumpStackTrace(void*)+0x64
  pc 0x000000010752ca54 fp 0x000070000fcced70 dart::Assert::Fail(char const*, ...) const+0x84
  pc 0x0000000107c59dcf fp 0x000070000fcceda0 Dart_DeleteWeakPersistentHandle+0xbf
  pc 0x0000000119985042 fp 0x000070000fccedc0 WeakHandle::finalize_(void*, void*)+0x16
  pc 0x0000000107c566c7 fp 0x000070000fccee00 dart::FinalizablePersistentHandle::Finalize(dart::IsolateGroup*, dart::FinalizablePersistentHandle*)+0x67
  pc 0x00000001076bed8d fp 0x000070000fccee60 dart::IsolateGroup::~IsolateGroup()+0xdd
  pc 0x00000001076bf8ed fp 0x000070000fcceea0 dart::IsolateGroup::Shutdown()+0x13d
  pc 0x000000010781bbe8 fp 0x000070000fccef30 dart::ThreadPool::WorkerLoop(dart::ThreadPool::Worker*)+0x138
  pc 0x000000010781c07d fp 0x000070000fccef60 dart::ThreadPool::Worker::Main(unsigned long)+0x5d
  pc 0x000000010778c67f fp 0x000070000fccefb0 dart::OSThread::GetMaxStackSize()+0xaf
  pc 0x00007fff205128fc fp 0x000070000fccefd0 _pthread_start+0xe0
  pc 0x00007fff2050e443 fp 0x000070000fcceff0 thread_start+0xf
-- End of DumpStackTrace
/Users/kasper/Projects/flutter/beta/bin/internal/shared.sh: line 223: 10943 Abort trap: 6           "$DART" "$@"

Obviously it is not needed to call Dart_DeleteWeakPersistentHandle during shutdown the isolate, since the lifetime is tied to the same, but how do I as a user distinguish this from the situation where the associated Dart_Handle has been garbage collected?

Could you expose Dart_CurrentIsolateGroup as Dart_CurrentIsolateGroup_DL so we can check, if it is safe to call Dart_DeleteWeakPersistentHandle.

[nielsenko]

Another solution would be to make it legal to call Dart_DeleteWeakPersistentHandle during shutdown of the isolate, as I believe was the original intend.

[blagoev]

Just so it is more clear what is the issue, here is a sample code.
createWeakHandle is called through FFI

//dart code

void main(List<String> arguments) {
   var obj = Object();
   createWeakHandle(obj);
}

//c code

Dart_WeakPersistentHandle myhandle;

void handle_finalizer(void* isolate_callback_data, void* somePrt) {
  Dart_DeleteWeakPersistentHandle_DL(myhandle);
}

void createWeakHandle(Dart_Handle handle) {
  myhandle = Dart_NewWeakPersistentHandle_DL(handle, somePrt, size, handle_finalizer);
}

//run the sample with dart
dart run

This will result in a crash on main method exit. This is also the same for Dart_DeletePersistentHandle

I believe Dart_DeleteWeakPersistentHandle and Dart_DeletePersistentHandle should be working everytime even on isolate teardown, or should be a no op in that case, since all Persistent and Weak handles should be deleted in the finalizer as they are not autodeleting as FinalizableHandle. My investigation in the Dart SDK repo shows that all samples/usages of these methods are leaking these handles (forgetting to delete them in the finalizer).

[mraleph]

/cc @dcharkes I believe I saw the same pattern in your CL which indicates that API is indeed problematic and we might need to change the API a bit.

[dcharkes]

will have a current isolate group

This is indeed not the behavior.

I'm only deleting handles when the isolate is not shutting down in that CL: https://dart-review.googlesource.com/c/sdk/+/229544/11/runtime/lib/finalizer.cc#69

[nielsenko]

Can you share a bit about the status of this?

[mraleph]

@dcharkes @mkustermann I think we need to fix this API, because it makes it somewhat complicated to use. As a temporary work-around we could expose Dart_CurrentIsolateGroup through the DL API - but maybe we should change shutdown process in a way that deleting handles becomes safe - so that users would not need to include checks in their own code. WDYT?

[nielsenko]

We would really appreciate that, even if it is just exposing Dart_CurrentIsolateGroup in the DL API.

Right now we are using an ugly hack that is based on the fact that Dart_WeakPersistentHandle is equivalent to Dart_FinalizableHandle, since they are both FinalizablePersistentHandle internally, and hence we can get a way with:

//TODO: We use FinalizableHandle since it is auto-deleting. Switch to Dart_WeakPersistentHandle when the HACK is removed
Dart_FinalizableHandle m_handle;
...
Dart_WeakPersistentHandle weakHnd = reinterpret_cast<Dart_WeakPersistentHandle>(m_handle);

Obviously we would like to avoid that.

[mkustermann]

we could expose Dart_CurrentIsolateGroup through the DL API

The concept of an isolate group doesn't exist on its own. It is created internally with the first isolate and deleted internally when the last isolate dies. So I don't think exposing it through the API is a very good idea.

shutdown process in a way that deleting handles becomes safe

My suggestion would be to perform finalization of persistent/weak-persistent handles with a valid isolate group, before triggering the isolate group shutdown itself. e.g. right here in runtime/vm/isolate.cc

const bool shutdown_group =
    isolate_group->UnregisterIsolateDecrementCount(isolate);
if (shutdown_group) {
  ...
    Thread::EnterIsolateGroupAsHelper(isolate_group, Thread::kUnknownTask,  /*bypass_safepoint=*/false);
    BackgroundCompiler::Stop(isolate_group);
    Thread::ExitIsolateGroupAsHelper(/*bypass_safepoint=*/false);
  ...
}

[mkustermann]

/cc @aam Interested in looking at this?