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.06k stars 1.56k forks source link

Isolate.kill() cancels earlier control events #28181

Closed alsemenov closed 7 years ago

alsemenov commented 7 years ago

The description for method Isolate.kill() reads:

void kill({ int priority: BEFORE_NEXT_EVENT }) Requests the isolate to shut down.

The isolate is requested to terminate itself. The priority argument specifies when this must happen.

The priority must be one of IMMEDIATE or BEFORE_NEXT_EVENT. The shutdown is performed at different times depending on the priority:

IMMEDIATE: The isolate shuts down as soon as possible. Control messages are handled in order, so all previously sent control events from this isolate will all have been processed. The shutdown should happen no later than if sent with BEFORE_NEXT_EVENT. It may happen earlier if the system has a way to shut down cleanly at an earlier time, even during the execution of another event. BEFORE_NEXT_EVENT: The shutdown is scheduled for the next time control returns to the event loop of the receiving isolate, after the current event, and any already scheduled control events, are completed.

The following sample code demonstrates, that ping event, which is send before kill(), is discarded.

import "dart:isolate";
import "dart:async";

entryPoint(SendPort sendPort) async {
  ReceivePort receivePort = new ReceivePort();
  sendPort.send(receivePort.sendPort);
  await for (var event in receivePort){
    print(event);
  }
}

Future ping(Isolate isolate, payload, [Duration timeout]) async {
  ReceivePort pingPort = new ReceivePort();
  Future result = pingPort.first;
  isolate.ping(pingPort.sendPort, response:payload);
  if (timeout!=null){
    result = result.timeout(timeout, onTimeout: () {
      pingPort.close();
      return "timeout";
    });
  }
  return result;
}

main() async {
  ReceivePort onExit = new ReceivePort();
  ReceivePort receivePort = new ReceivePort();
  Isolate isolate = await Isolate.spawn(entryPoint, receivePort.sendPort, onExit:onExit.sendPort);
  SendPort sendPort = await receivePort.first;

  Future pingFuture = ping(isolate, "ping", new Duration(seconds:2));

  isolate.kill(priority:Isolate.IMMEDIATE);

  print(await pingFuture);
  await onExit.first;
  print("test finished");
}

Output:

λ d:\dev\dart\dart-sdk\bin\dart test6.dart timeout test finished

Expected output:

λ d:\dev\dart\dart-sdk\bin\dart test6.dart ping test finished

Unfortunately, the sample code is not stable enough. For example if function ping is inlined test passes. Also it passes with isolate.kill(priority:Isolate.BEFORE_NEXT_EVENT); but more complex scenarios with BEFORE_NEXT_EVENT fail.

Dart VM version: 1.21.0 (Wed Dec 07 06:52:21 2016) on "windows_x64"

lrhn commented 7 years ago

Your ping helper method is async. That means that its body isn't executed immediately, but is instead scheduled for a later microtask. Right after calling ping, you call isolate.kill, so the kill message is actually sent before the ping message. That's also why it works if the ping function is inlined - then the ping is sent before the kill. Since the function doesn't need to be async, I'll wager that if you remove the async marker, the test will succeed.

I'll take this as another vote for #22909.

alsemenov commented 7 years ago

You are right, the root cause is async marker for method test. I have removed it and test passes. So this is my mistake, I forgot that async methods are executed later.

However, some error message, that ping is sent to already killed isolate would be helpful in understanding what is wrong.

lrhn commented 7 years ago

The Isolate object could maintain state to know that kill has been sent, and throw if you try to do something after a kill. It's not that simple, though - since a kill operation might fail if you don't have the correct capability, and one way to test if it failed is to request a ping after the kill and see if you get a response.

All in all, the Isolate class is a very low-level API, really just the raw way to send control messages to another isolate. I don't think it's a good idea to try to make it much fancier, because it's still going to be low-level, now just with a few odd higher-level features bolted on. Instead we should make sure that users have better, probably more specialized, higher level APIs build on top of the Isolate class, like the IsolateRunner from package:isolate.