I think this should be more like what is happening in CreateRootIsolate where the embedder data is created before the isolate. I.e.,

auto* isolate_group_data =
      static_cast<std::shared_ptr<DartIsolateGroupData>*>(
          Dart_CurrentIsolateGroupData());

UIDartState::Context context ...
auto child_isolate_data = std::make_unique<std::shared_ptr<DartIsolate>>(
      std::shared_ptr<DartIsolate>(new DartIsolate(*isolate_group_data)->GetSettings(),  // settings                                                                                                                             
                                                   true,      // is_root_isolate                                                                                                                      
                                                   context    // context                                                                                                                              
                                                   )));
Dart_CreateIsolateInGroup(parent_isolate, ...,
                          child_isolate_data.release(), // owernship passes to VM
                          &error);

because in this case the engine is creating the isolate. InitializePlatformIsolate is the reactionary case of the VM creating the isolate.

Done. But when I set the child_isolate_data in Dart_CreateIsolateInGroup to a non-null value, the platform isolate never shuts down. @chinmaygarde Any idea why that would be?

I think you would need something in the message callback epilogue to notice the isolate is done. I think this doesn't happen for the UI isolates because the UI isolates have event sources other than receive ports.

MessageNotifyCallback() {
  pending_messages++;
  platform_task_runner.PostTask(OnMessage);
}

OnMessage() {
  ...
  pending_messages--;
  Dart_HandleMessage();
  if (pending_messages == 0 && !Dart_HasLivePorts()) {
    Dart_ShutdownIsolate();
  }
}

This is missing the copy-and-validate checking that happens in the regular Isolate.spawn case (the use of CopyMutableObjectGraph in Isolate_spawnFunction). There's not an API function for it, but sending the entry-point and initial message via a port would provide that.

void _platformIsolateMain(SendPort replyPort) {
  port = new RawReceivePort();
  port.handler = (entryPointMessage) {
     port.close();
     entryPointMessage[0].call(entryPointMessage[1]);
  };
  isolate = Isolate.current;
  replyPort.send([isolate.controlPort, isolate.pauseCapability, isolate.terminateCapability, port.sendPort]);
}

Done.

UIDartState::Current()->GetTaskRunners()->GetPlatformTaskRunner() ?
(before exiting the parent isolate)

I was using the global_platform_task_runner hack because the task runners I was getting from the UIDartState were often null. While doing the other refactors you suggested, I realized that's because the child isolates are being initialized using null_task_runners. So yeah, this is no longer necessary if I initialize the UIDartState::Context with real task runners.

bool isRunningOnPlatformThread() {
 return GetTaskRunners().GetPlatformTaskRunner().RunsTasksOnCurrentThread();
}

?

That's definitely better, but it's still exposing a very internal bit of engine machinery to the Dart PlatformIsolate API. Maybe when I write the test for real I should FFI back into C++ land and check the thread from there.

I would expect shutdown of platform isolates to happen during shutdown of a Flutter engine instance (Engine/Shell/RuntimeController).

DartVM is a process-wide object that will remain alive after engine shutdown if at least one other engine instance is still active. Or in some configurations the DartVM may stay alive for the entire process lifetime (see Settings::leak_vm)

Done, but I'm not sure how to access the RuntimeController from DartIsolate::OnShutdownCallback() on the platform isolate. platform_configuration() is only non-null on the root isolate.

Is there a race between this and the platform thread potentially calling ShutdownPlatformIsolates?

RegisterPlatformIsolate has an assertion that assumes that is_shutdown_ is false. But IIUC it's possible for shell shutdown to call ShutdownPlatformIsolates on the platform thread between the platform_isolate_manager->IsShutdown() call above and this point.

Oh right. I've tightened up this sequence. I think it should be fixed now.

RegisterPlatformIsolate now double checks is_shutdown_, and returns false if it is shutdown. In that case the caller stops setting up the isolate and does nothing else (ie doesn't explicitly shut it down). This is fine because this case means that the engine is being shutdown, so the isolate is about to be shutdown anyway.

RemovePlatformIsolate and ShutdownPlatformIsolates are only ever called on the platform thread, so we don't have the same race condition between them.

I've added comments about all this stuff too.

Would it make things simpler if spawning of platform isolates was only allowed from within the root isolate?

Yeah, this would be a lot simpler. I suppose this is a reasonable restriction, at least for now.

I think this is very dangerous logic, making strong assumptions.

Imagine the caller does this

Dart_EnterIsolate();
Dart_EnterScope();
... result = HandleMessage();
... OnMessageEpilogue(result);
Dart_ExitScope();
Dart_ExitIsolate();

If the OnMessageEpilogue() shuts the isolate down, the Dart_ExitScope() will segfault the program.

So you're assuming here:

• The caller handles isolate death after the OnMessageEpilogue() call appropriately
• The VM allows Dart_ShutdownIsolate() with actively entered handle scopes by embedder

It may be that this is guaranteed. If it is, it needs to have a comment explaining that here and on OnMessageEpilogue interface declarations all the way up to the caller that provides the guarantee.

I think a much cleaner way would be if this OnMessageEpiloge() would return a bool that tells the caller that it should shutdown the isolate. That bool can be returned all the way to the message loop handler, which is the outermost place where Dart_EnterIsolate() was called, that's where Dart_ShutdownIsolate() should also belong.

I think this responsibility belongs into the message handler loop: The message handler runs for the platform isolate, handling all messages. After all messages are handled, it checks whether there's no longer ports open, if so, it should shutdown the isolate.

I can see that some logic already exists for that in third_party/tonic/dart_message_handler.cc:

void DartMessageHandler::OnHandleMessage(DartState* dart_state) {
  DartIsolateScope scope(dart_state->isolate());
  DartApiScope dart_api_scope;
  ...
    // We are processing messages normally.
    result = Dart_HandleMessage();
    ...
    dart_state->MessageEpilogue(result);
    ...
  if (error) {
    UnhandledError(result);
  } else if (!Dart_HasLivePorts()) {
    // The isolate has no live ports and would like to exit.
      isolate_exited_ = true;
  }
  ...
}

void DartMessageHandler::UnhandledError(Dart_Handle error) {
  ...
  if (Dart_IsFatalError(error)) {
    ...
    Dart_ShutdownIsolate();
  }
}

I'm a little bit puzzled that the isolate_exited_ = true is being set but it doesn't call Dart_ShutdownIsolate() here.

Basically the logic you've written here should belong into the message handler: It should remember whether there's pending messages, and if there's no more pending messages and no open receive port, it should shut the isolate down.

It may be that the UI isolate isn't using this path, because it will keep the UI isolate alive even if there's no open ports and no pending messages (as the flutter engine basically artificially injects drawFrame messages - which doesn't use the port mechanism).

I think this comment and the one above would make this much cleaner. It could be done as a separate PR before this lands.

You may consider having 2 methods, so it's obvious on the call site if the caller gets a racy return value.

// Only callable on the platform thread.
bool HasShutdown() {
  ASSERT(current-thread-running-on-platform-task-runner);
  ...
}

// If returns true guaranteed to be shutdown, if false it may or may not have shutdown.
bool HasShutdownWithFalseNegativePossibility() {
   ...
}

ping :)

Done.

Then consider renaming to IsRegisteredForTestingOnly :)

(mentioned before) Not a native speaker (so maybe get second opinion) but to me it seems we should use the "running on a thread" terminology instead of "running in a thread" (i.e. consistently rename in -> on in the PR).

I'd also prefer using "on" in these comments and calling the API runOnPlatformThread

Done.

When we later allow calling this from a helper isolates, we may want to

  // When scheduling a computation
  if (_pending.isEmpty) {
    receiver.keepIsolateAlive = true;
  }
  _pending[nextId++] = ...

  // When getting result
  ... = _pending.removeAt(id);
  if (_pending.isEmpty) receiver.keepIsolateAlive = false;

Consider leaving a comment

(the keepIsolateAlive getter/setter can be optimized to not go to runtime)

Not really related to spawning, is it? Maybe UnsupportedError

Removed, per your other comment

Why is this method generic?

Leftovers from the refactor.

This is a bit hard to understand, consider instead letting the platform isolate itself attach the exit listener by giving it the Isolate as part of the closure.

  final currentIsolate = Isolate.current;
  final sendPort = receiver.sendPort;
  try { 
    _nativeSpawn((SendPort sendPort) => _platformIsolateMain(currentIsolate, sendPort), sendPort);
  } ... {}

_platformIsolate(Isolate uiIsolate, SendPort uiIsolatePort) {
  uiIsolate.addExitListener(...);
}

Done.

Are we actually registering an exit handler (don't see where)?

Either we should throw an error if we think this cannot happen or we should just complete all pending RPCs with an error - but not silently ignore it.

Jason's comment mentions how to make a private test-only API, so I can now implement and test this logic.

Should this be a getter? (also in -> on)

Done

This is passing the sendport to C++, capturing it in lambda, passing it back into Dart on the new isolate.

You can avoid this complexity by making the dart side capture this

  final uiIsolate = Isolate.current;
  final uiIsolateSendPort = ...;
  _nativeSpawn(() => _platformIsolateEntryPoint(uiIsolate, uiIsolateSendPort));

then you invoke entry_point here without any arguments.

Done.

Could you explain how this can possibly happen? If it can't, it'd be better to assert it.

No longer possible now that the PlatformIsolate.spawn method is removed.

Please add bullet points to the issue for any postponed TODOs

Done.

I'd also prefer using "on" in these comments and calling the API runOnPlatformThread

What are the use cases for making this a public API?

If this API is only used by tests to verify that the code is running on the right thread, then I'd prefer not to add it to the engine's public API.

Can this instead be a native function that is exported to tests by https://github.com/flutter/engine/blob/main/shell/testing/tester_main.cc and then invoked via FFI? This pattern is used by some other Flutter Dart tests.

Add a new DartIsolate constructor that takes the PlatformIsolateManager as an argument and sets the is_platform_isolate_ flag.

This avoids the need to access the private platform_isolate_manager_ field here and allows removal of is_platform_isolate from the original DartIsolate constructor.

Done

PlatformIsolateManager should be held in a std::shared_ptr within RuntimeController/DartIsolate. It should be captured as a std::weak_ptr in the tasks queued to the platform thread.

If engine shutdown happens before a platform thread task executes, then the RuntimeController will be destructed before the task runs. The tasks queued by DartIsolate::CreatePlatformIsolate can detect this safely with a weak_ptr.

2024