Fix process leaks in tool tests #51382

dnfield · 2020-02-25T01:43:03Z

The integration tests in the tool invoke Flutter to:

Create project(s) in temp directories
Run those projects directly via flutter_tester
Make sure that you can attach a debugger, evaluate expressions, etc.

These processes leak. The shell script/batch script files are not set up to wait for the dart process they invoke to finish, and thus they can exit (and complete the process.exitCode future) before the Dart VM process they fork actually finishes. On Windows, this is particularly bad, since the leaked process(es) end up holding an exclusive lock on the temp directory, meaning we fail to clean it up. We've been swallowing that under the assumption that it was a permissions issue, but it appears to not be a permissions issue and just be an issue of leaked processes.

This patch adds some additional waits/attempts to terminate the Dart VM(s) that the tests spawn. In cases where we cannot be sure that the spawned VM has terminated, it adds some additional OS specific waiting to check that the PID the VM created is no longer active (on Windows in tasklist and on POSIX in response to kill -0).

We should be able to further improve the test_driver logic if we can get dart-lang/sdk#40759 resolved. Alternatively, we could look at making flutter.bat wait for Dart to exit when it launches, but I'm a bit scared to touch that logic since it has some comments about how it has to work in a bunch of scenarios I'm not sure we have tests for (/cc @goderbauer). The Dart feature would be useful regardless of this particular instance of usage though.

fluttergithubbot · 2020-02-25T01:43:06Z

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.

dnfield · 2020-02-25T04:06:10Z

packages/flutter_tools/test/src/common.dart

-  } on FileSystemException catch (error) {
-    print('Failed to delete ${directory.path}: $error');
-  }
+  directory.deleteSync(recursive: true);


I'd like to leave this for now in case we see flakes around this pop up again. Should I add a TODO to remove it?

The actual reason these files were failing to delete was because we had processes that weren't being killed before trying to delete files in use, which on Windows causes exceptions.

Could you link to the Dart SDK bug?

I'm not sure if that's the right bug to link here - if anything I'd say file a new bug to track removing this code once we're convinced the current change hasn't re-introduced old flakes.

jonahwilliams · 2020-02-25T04:12:33Z

packages/flutter_tools/test/integration.shard/test_driver.dart


+  // TODO(dnfield): This is racy. If dart-lang/sdk#40759 can be resolved, we
+  // should remove this.
+  Future<void> _ensureDead(int pid, int tries) {


uber nit: shouldn't be called ensureX if it can't ensure it.

Renamed to tryWaitForPidDeath, and moved it to test_utils.dart so it can be reused in the daemon_mode_test for the same reason.

jonahwilliams · 2020-02-25T04:34:00Z

This seems reasonable to me, but I'd wait for @zanderso to take a look too

zanderso · 2020-02-25T05:16:11Z

Initial thoughts:

Please expand the PR description to explain what problem this is solving (or link to the github issue), and why it solves it, and if there are any remaining issues.
I think we need to take a step back and think about what is being bought by going through the shell script indirection here rather than invoking Dart directly. It seems like if the test driver had a Process object for the VM, then some difficulty here could be avoided, and we wouldn't need a feature request against killPid.

dnfield · 2020-02-25T05:17:32Z

dev/ci/docker_linux/Dockerfile

  ca-certificates \
-  gnupg
+  gnupg \
+  procps


@christopherfujino - should I split this into a separate PR?

This is needed so that kill is available in the container.

dnfield · 2020-02-25T05:24:55Z

@zanderso - I updated the PR description.

The closest bug I can think of to this is #29141. The resolution of that issue was to make an outer harness just kill processes at the end of it. We can't quite do that in these tests.

As to whether it would be good enough to avoid the flutter.bat/flutter shell scripts, I'd leave that to @Hixie - I believe he wrote most of these tests.

dnfield · 2020-02-25T05:27:48Z

We also might still have a real bug such that in certain debugging scenarios, the Windows VM leaks. That seems to be the case in these tests, without explicitly killing it.

dnfield · 2020-02-25T06:06:51Z

Hm. This seemed to work locally on a Windows box, but I think the Windows instance we're using is just slower... Not sure why the process is still sticking around.

dnfield · 2020-02-25T18:16:04Z

Discussed offline. This approach will still be flaky, and it's not clear that it's any less flaky than the current approach (even though it may avoid flakes in many common cases).

I'm going to file an issue specifically about some of the things I've discovered here. I'll leave this open for now - hopefully @Hixie might have some good input on this as the author of many of these tests.

dnfield · 2020-03-03T05:31:27Z

I'm going to close this - it's not ready as is and I don't have a good sense of when it will be.

roll engine plus fix leak

eee9f24

fluttergithubbot added engine flutter/engine related. See also e: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Feb 25, 2020

googlebot added the cla: yes label Feb 25, 2020

one more test

e2613f0

dnfield added team: flakes and removed engine flutter/engine related. See also e: labels. labels Feb 25, 2020

dnfield commented Feb 25, 2020

View reviewed changes

dnfield changed the title ~~roll engine plus fix leak~~ Fix process leaks in tool tests Feb 25, 2020

dnfield requested review from jonahwilliams and zanderso February 25, 2020 04:08

whitespace

a6cbeff

jonahwilliams reviewed Feb 25, 2020

View reviewed changes

dnfield added 2 commits February 24, 2020 20:25

more scientific naming

00db27f

more waiting, remove unused import

bca50f3

dockerfile, stray print

1764479

dnfield requested a review from christopherfujino as a code owner February 25, 2020 05:15

dnfield commented Feb 25, 2020

View reviewed changes

fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Feb 25, 2020

dnfield added 2 commits February 24, 2020 21:44

die die die

91a5051

just print to test for CI

138f8d1

dnfield mentioned this pull request Feb 25, 2020

The tool integration tests are leaky and need significant work to fix #51421

Closed

simplify

4fa3e93

Merge remote-tracking branch 'upstream/master' into no_leak

ca9c357

dnfield closed this Mar 3, 2020

github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021

Fix process leaks in tool tests #51382

Fix process leaks in tool tests #51382

Uh oh!

Conversation

dnfield commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fluttergithubbot commented Feb 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanderso commented Feb 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Feb 25, 2020

Uh oh!

dnfield commented Feb 25, 2020

Uh oh!

dnfield commented Feb 25, 2020

Uh oh!

dnfield commented Feb 25, 2020

Uh oh!

dnfield commented Mar 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dnfield commented Feb 25, 2020 •

edited

Loading

jonahwilliams commented Feb 25, 2020 •

edited

Loading