Resume isolate before terminating tests to prevent flutter_tester leaks in integration tests

[DanTup]

When running integration tests locally I noticed that flutter_tester processes were being leaked. This is because during some of the tests the Flutter isolate is left paused and when we try to tear down (by sending SIGTERM) the output of the process says

Waiting for current test to finish
Press Ctrl+C again to terminate

Since the process doesn't quit within 10s, we then send SIGTERM which terminates the process and no cleanup is performed (so the child flutter_tester process is left). Sending a second SIGTERM did not resolve the issue in my testing because it also resulted in an unclean shutdown.

This probably fixes #26350, though I don't think it's a perfect fix since other code that terminates while paused (for example the user clicking Stop in the IDE while at a breakpoint) could still leak these processes - I don't think that's really a Flutter issue though (it's likely a package:test issue, or limitation that the IDEs should consider).

[fluttergithubbot]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[christopherfujino]

indentation?

[zanderso]

lgtm w/ indentation fix.

[christopherfujino]

tools_tests are failing, can you try rebasing on ToT? this might have been fixed upstream.

[DanTup]

Hmm, the failure does seem to be caused by this change - but I don't understand why the try/catch seems to be being ignored here and the error still thrown:

The same code in a simple command line app seems to behave as I'd expect (the catch block executes). Is this a bug or am I overlooking something?

[DanTup]

I've opened dart-lang/sdk#39513 about the caught exception that's still failing the test (it's possible it's my bug, but I can't come up with an explanation that makes sense to me).

I'll see if I can find a better way to work around it here for now.

[DanTup]

I've rebased on latest code, fixed the indenting and also added a null check to avoid trying to resume when there's no VM service. The tests seem to all pass locally, though I'm still confused by the behaviour of the catch block - I may come back and tweak this in a future PR based on the response to dart-lang/sdk#39513.

[zanderso]

If resume().timeout(defaultTimeout) might leak an error into the containing Zone (via an unawaited Future or microtask), you can use asyncGuard() to make sure it is caught where you want it to be caught.

However if resume().timeout(defaultTimeout) is in the tool codebase, the preferred solution is to not leak the error.

[DanTup]

I was able to reproduce it without any zones in dart-lang/sdk#39513 (and without the timeout firing). It feels like a bug, but I'm not sure 🤷‍♂️

[zanderso]

I left a comment on dart-lang/sdk#39513.

[DanTup]

I pushed 5207952 which changes the timeout code (which aimed to print a warning when an action took too long, including all of the messages that went over the API in the meantime if they're not already being printed) to use a Timer which is cancelled if the future completes.

This seems to resolve the issue - exceptions in resume are now handled and don't fail tests.

[DanTup]

Looks like Cirrus builds aren't running for this PR again 😞

@fkorotkov - could be another instance of 404s from GH?

[fkorotkov]

@DanTup there was another issue this time. I've poked around and found that GH timed out while delivering the webhook. Cirrus' metrics looks fine around that time so will investigate more to see on which end this happened. In the meantime I've manually redelivered the webhook.