Do we actually need to block the isolate?

If there is still work to be done, the isolate should not exit. If there is no work to be done, exiting without printing the "async test done" message would be is a perfectly good way to fail the test, instead of hanging.

I tried removing the port from async_helper.dart, and testing the lib_2 tests on dartk/vm still succeeds without hitch.

I would interpret this bug as: "address our testing needs that hides the dependency on isolates".

I.e., if we don't need to block the isolate to do the above, then that's not a requirement.

I would interpret this bug as: "address our testing needs that hides the dependency on isolates".

precisely, that's basically what I am going after.

Do we actually need to block the isolate?

I'm not 100% sure, but my understanding is that we do need it. I'm not sure what scenarios trigger this but my understanding is that without blocking the VM might exit too early. That might be due to external callbacks that the VM doesn't have a way to know if they have pending work, or some other reason I don't know of.

Adding some people that might know the answer: @a-siva @mkustermann @peter-ahe-google

One of the reason for this functionality is: It provides an ability to test low level implementation details. For example to test the implementation of Future.then, await/async, ... The VM will ensure it only shuts down an isolate once the last port is closed. So a faulty implementation will cause a timeout (instead of e.g. silently exiting early on -- we actually had some of those bugs in the VM implementation of await/async).

Another reason is that a lot of our tests, in particular corelib/io/... tests, were not "allowed" to use other packages, e.g. package:test, because those other packages use the very same functionality the tests are testing. So there is a need to ensure asynchronous code, which is expected to execute, is executed.

IIRC, For dart2js we have introduced the unittest-suite-wait-for-done and unittest-suite-{fail,done}. The test runner intercepts those prints and will not mark the test as done until one of the fail/done markers was received. Maybe something like that could be ported to the VM?

Personally, I'm not a particular fan of introducing more special dart: libraries, even for testing purposes.

Maybe @peter-ahe-google can provide his view on this as well?

@sigmundch suggests a feature would could call "keep-live" (it generalizes what we doing with new ReceivePort()). Unfortunately, this doesn't work to test at least one feature, the keep-alive feature itself.

@vsmenon suggests that we don't need this feature because the VM should just not exit if it has more work to do. Unfortunately, that's the behavior we're trying to verify is working. So we need a way to write tests of this feature.

We need to remember that we're testing the features of the platform (a service for developers working on the Dart SDK), not providing a testing framework for Dart (a service for developers using Dart).

The above two mentioned suggestions suffer from the same problem: they aren't fail-safe (if they fail they may cause harm as we won't notice they're broken and other things might break). This is why we added the unittest-suite-wait-for-done mechanism.

We've spend many hours debugging problems that turned out to be code not being run because an async program exited prematurely. This is why we care so much about this.

Here's an alternative solution:

• We rename all tests that use package:async_helper so they all end with _async_test.dart.
• We change test.dart (aka test.py) to not directly invoke these tests, but instead invoke them via a generated file:

foo_test.dart:

import 'foo_test_async.dart' as wrapped;

main(arguments, other) {
  wrapped.main(arguments, other).then((_) {
    print("unittest-suite-done"); // *1*
  });
}

Notice that the line indicated by *1* could be different for each runtime or compiler.

The tests using async_helper don't all have asynchronous main methods (most don't, since that has until recently been a problem due to the delayed start of async functions, which meant that it wouldn't print "unittest-suite-wait-for-done" soon enough), so I'm not sure the wrapping will just work.

Apart from that, it seems to be exactly what async_helper does (except that it also keeps the isolate alive, for no apparent reason) - it ensures that we print the "unittest-suite-wait-for-done" when we start and the "unittest-suite-done" when the program completes. It uses no features fancier than a static integer variable for the counter.

The first call to asyncStart (when counter is zero) prints "unittest-suite-wait-for-done", and when there have been as many asyncEnds as asyncStarts (counter is back to zero), it prints "unittest-suite-done" and stops working.

If the program terminates before that, it's a failure, which is what we want. There is no need to keep the isolate alive and get a time-out, early termination is also an error.

Are there any other uses of async_helper than this? That is, is it used by any testing which doesn't check the printed lines, but relies on the "keeping alive" part? If not, I think we should just remove the dependency on Isolate and keep using async_helper like we already do.
(E.g. https://dart-review.googlesource.com/c/sdk/+/55662)

@lrhn we've gotten push-back on printing those messages. I think the wrapping could solve that as it enables us to use different features for each runtime or compiler.

Just clarifying the title for this bug - i.e., the dart stable blocker.

Is @lrhn 's CL sufficient to fix that?

Do we need a separate bug to discuss blocking the VM and / or printing?

@vsmenon @lrhn When looking at the test.dart code, it seems like like test.dart checks for unittest-suite-done in the VM case already, so Lasses's CL might be enough.

@mkustermann yes. Lasse's CL is certainly enough to close this issue.

Fantastic! Thanks everyone for all the input and detailed discussion!

@lrhn 's CL landed, so I think we can close.