#2398. Make asyncStart/End() safe in LibTest/async

[sgrekhov]

The most part of the changes here is to add asyncStart/End() guards to tests like

test(...) {
  asyncStart();
  future.then(() {
  ...
  asyncEnd();
  });
}

main() {
  asyncStart(); // added
  test(...);
  test(...);
  asyncEnd(); // added
}

Without these new guards if, for some reasons, asyncStart/End() pair in the first invocation of test() will be completed before the call of the second test() then the test will fail (invocation of asyncStart() after unit-test-suite-success throws) or may be successfully finished on web (if the driver will be fast enough)

There are also a little bit of code formatting, but not much. I'm going to run dart fromatter on these tests after merge of this PR

[eernstg]

LGTM!

I still think this style is somewhat misleading:

main() {
  asyncStart();
  test(Zone.current);
  test(Zone.current.fork());
  asyncEnd();
}

(because we aren't pairing up the two asyncStart/asyncEnd invocations with each other at run time), but I think we have been digging deep enough into this topic to be happy about what we've got now. ;-)

I'll await further input.

[osa1]

LGTM, thanks.

[osa1]

(because we aren't pairing up the two asyncStart/asyncEnd invocations with each other at run time), but I think we have been digging deep enough into this topic to be happy about what we've got now. ;-)

@eernstg Could you elaborate on what's wrong with the example you give? Is it from an actual test?

[eernstg]

Could you elaborate on what's wrong with the example you give? Is it from an actual test?

It's from LibTest/async/Zone/createPeriodicTimer_A01_t01.dart.

I don't think there is an actual problem, and certainly no need to make changes to working tests.

However, it did take some time for me to get used to the idea that we have a syntactic structure that seems to imply pairing (a parenthesis structure), but the actual run-time behavior has a different structure. In particular, here is the example that I mentioned:

main() {
  asyncStart();
  test(Zone.current);
  test(Zone.current.fork());
  asyncEnd();
}

When this test is executed, the asyncStart() in main will increase _asyncLevel from 0 to 1. I like to think about this as "asyncStart() pushes one element unto the asyncStartEnd stack, and we'll make sure that there is an asyncEnd() that pops it off again".

Next, test is a function that runs asyncStart() and introduces an asynchronous computation (which will run asyncEnd() at some point in the future, after main has returned). We invoke test twice, so we've pushed an asyncStart() three times, and introduced two asynchronous computations.

Next, asyncEnd() in main pops one element from the stack (but it isn't the element which was pushed by the asyncStart() which is visible in the same function body, it's actually the asyncStart() that occurred during the second invocation of test).

If test hadn't used asyncStart() or asyncEnd() then the asyncEnd() in main would have emptied the stack, and then we would have incurred a test failure later on, because all testing is supposed to have ended when the stack is empty.

So it's crucial that test does call asyncStart(), and also that it calls asyncEnd() in a callback (not at the top level of the function body of test), because that's needed in order to avoid the "I thought you were done already?"-error during the asynchronous computations.

Also, this means that the asyncStart() in main is paired up with the asyncEnd() in the callback of the 2nd invocation of test (or maybe the first one, we don't know exactly what the scheduler will do).

In summary, there's no way the nice symmetry of having both asyncStart() and asyncEnd() at the top level of the body of main will help us understand what's going on, and we even have to be so lucky that we remember to put asyncStart() and asyncEnd() into test. In particular, a lone asyncStart() and asyncEnd() in main (and none in test) would cause the entire test run to fail.

I tend to think that it would be more readable if we had an asyncStart(2) in main, and a corresponding asyncEnd() in the callback in test, and no more. The extra occurrences of asyncStart() and asyncEnd() that we actually have are just looking nice, and creating misleading hints. ;-)

In any case, I certainly don't want to block things at this point. So I'll land this PR now, and then we can always continue the discussion and make changes if there is a need for that.

[sgrekhov]

@eernstg thank you! Indeed, with the pair of asyncStart/End() in main we still may have a flaky test failure in case if test() are async functions. If the firsttest() and execution on main will be completed before start of execution of the second test() the thest may fail. So, now I really do think now that the tests must be written in the following way

test(...) {
  future.then(() {
  ...
  asyncEnd();
  });
}

main() {
  asyncStart(2); 
  test(...);
  test(...);
}

This should really prevent from failures caused by a threads race. Probably it doesn't make sense to update all of the tests right now because we don't have this problem in practice, but, at least, now we have understanding how such test should be written

[eernstg]

the tests must be written in the following way

Agreed! This whole discussion has certainly improved my understanding of how to properly use asyncStart() and asyncEnd(). That's great, thanks @osa1 and @mkustermann for your input on this!