Terminate non-detached test devices on `flutter run` completion #159170

matanlurey · 2024-11-19T21:32:10Z

Before this PR, it appeared we were accidentally leaking (keeping active) flutter_tester instances (or any test device) after flutter run completion, even if the runner was not explicitly detached. I think this is a bug, but I'll check with the tools team and possibly @jonahwilliams before finalizing this.

/cc @jason-simmons

packages/flutter_tools/test/general.shard/run_hot_test.dart

andrewkolos

LGTM

jason-simmons

I suspect that the original goal of HotRunner.cleanupAfterSignal was that a Ctrl-C of flutter_tools should not try to stop the app if the user connected to an already running app using flutter attach.

But it looks like cleanupAfterSignal intended to have Ctrl-C stop the app if the app was launched by flutter run. However, that does not actually happen because HotRunner.run calls HotRunner.attach, which sets the _didAttach flag. So a Ctrl-C goes down the appFinished code path instead of exitApp for both flutter attach and flutter run.

Having Ctrl-C stop the app on a flutter run would be similar to the behavior of the flutter_tools quit command. Pressing q stops the app after a flutter run but not a flutter attach.

Can anyone confirm whether Ctrl-C is expected to stop an app launched by flutter run?

If not, then it may be simpler to always have cleanupAfterSignal call appFinished and remove the exitApp code path.

jason-simmons · 2024-11-20T00:18:45Z

Alternatively, it might make sense to have a flag that represents whether the app connection was initiated through flutter run or flutter attach.

cleanupAfterSignal could then use that flag to decide whether to call appFinished or exitApp. (I'm guessing that this is what _didAttach was originally supposed to represent).

With that change, Ctrl-C will stop an app that was launched with flutter run. And that will also accomplish the goal of having flutter_tools stop the flutter_tester process if the tool was launched with flutter run -d flutter-tester and is then killed by a signal.

confused

andrewkolos · 2024-11-20T17:39:45Z

Can anyone confirm whether Ctrl-C is expected to stop an app launched by flutter run?

I don't believe this is documented. In fact, the current behavior differs based on target platform. When running a Flutter app on an Android emulator, Ctrl-C leaves the app running (unlike sending q which exits it). When running a Flutter app on desktop (macOS), Ctrl-C exits the app.

I don't attach/detach when working on Flutter apps personally, so I don't know if my opinion is worth anything. Personally, I would expect Ctrl-C to behave similarly to sending q. However, q also behaves ambiguously. q does not quit the app during flutter attach, as Jason mentioned, even though the description text still says the app will be. Regardless, this can be treated as a separate issue (as it already is, see #122303).

andrewkolos · 2024-11-20T17:41:00Z

TL;DR Jason's last comment sounds good to me

matanlurey · 2024-11-20T18:11:18Z

Alternatively, it might make sense to have a flag that represents whether the app connection was initiated through flutter run or flutter attach.

cleanupAfterSignal could then use that flag to decide whether to call appFinished or exitApp. (I'm guessing that this is what _didAttach was originally supposed to represent).

With that change, Ctrl-C will stop an app that was launched with flutter run. And that will also accomplish the goal of having flutter_tools stop the flutter_tester process if the tool was launched with flutter run -d flutter-tester and is then killed by a signal.

Sounds good, moving this back to draft - will ping when updated.

matanlurey · 2024-11-20T19:33:00Z

Ok, this simplifies things a bit and added another test case!

jason-simmons

Overall LGTM

jason-simmons · 2024-11-20T20:34:28Z

packages/flutter_tools/lib/src/resident_runner.dart

+  /// - [attach] is used to explicitly connect to an already running app.
+  @protected
+  @visibleForTesting
+  bool isDetached = false;


I think the name of this variable can be misleading because running the flutter attach command will cause isDetached to be set to true.

Trying to think of a better name - maybe something like stopAppDuringCleanup or appWasStartedByRunner?

SGTM, will do before merging. I like stopAppDuringCleanup.

_attachWithoutMarkingDetached could probably use a rename then, as well. I'm trying to think of a better name...

_attachWithoutStoppingAppOnDetach?

I tweaked again in rename _attachWithoutStoppingAppOnDetach to _attachWithoutStoppingAppDuringCleanup. A little bit more verbose but precise—I think.

Works for me.

Or perhaps name it _attach or _attachToApp or _connectToApp?

This method does the work of attaching to the app that is common between flutter run and flutter attach.

I think _attachWithoutStoppingAppDuringCleanup makes it sound like it clears the stopAppDuringCleanup flag, which is not the case.

Tweaked again in rename _attachWithoutStoppingAppDuringCleanup to _attach

Closes #158560. I believe but am not sure as of #159170 merging, many process flakes that were consuming memory and in turn, making Gradle particularly sensitive to timing out or being killing by the OS for low-memory, have been rectified. It is possible there are additional problems, but they aren't visible at the moment. I'd like to re-enable these and keep tracking their stability.

…on (flutter/flutter#159170)

Roll Flutter from 8536b96 to 93d772c (37 revisions) flutter/flutter@8536b96...93d772c 2024-11-21 [email protected] Added additional logging to `_listCoreDevices` (flutter/flutter#159275) 2024-11-21 [email protected] Roll Flutter Engine from 78b87f3fe023 to d1a08064e193 (1 revision) (flutter/flutter#159280) 2024-11-21 [email protected] Shut down DevTools and DDS processes if flutter_tools is killed by a signal (flutter/flutter#159238) 2024-11-21 [email protected] Remove `RepaintBoundary` that is no longer needed. (flutter/flutter#159232) 2024-11-21 [email protected] Try a speculative fix for Gradle OOMs. (flutter/flutter#159234) 2024-11-21 [email protected] On-device Widget Inspector button exits widget selection (flutter/flutter#158219) 2024-11-21 [email protected] Roll Flutter Engine from 523d381893c8 to 78b87f3fe023 (2 revisions) (flutter/flutter#159270) 2024-11-21 [email protected] Remove `firebase_abstract_method_smoke_test` (flutter/flutter#159145) 2024-11-21 [email protected] Roll Packages from e95f6d8 to 913b99e (7 revisions) (flutter/flutter#159268) 2024-11-21 [email protected] Roll Flutter Engine from 69c325513a65 to 523d381893c8 (3 revisions) (flutter/flutter#159263) 2024-11-21 [email protected] [flutter_tools] opt iOS/macOS apps out of Metal API validation via migrator, update templates in repo. (flutter/flutter#159228) 2024-11-21 [email protected] Scribe Android handwriting text input (flutter/flutter#148784) 2024-11-21 [email protected] Terminate non-detached test devices on `flutter run` completion (flutter/flutter#159170) 2024-11-21 [email protected] Un-skip tests that use `flutter build apk`. (flutter/flutter#159231) 2024-11-20 [email protected] Roll Flutter Engine from 2d32cf3a7971 to 69c325513a65 (1 revision) (flutter/flutter#159229) 2024-11-20 [email protected] Roll Flutter Engine from 3828681d1f86 to 2d32cf3a7971 (3 revisions) (flutter/flutter#159226) 2024-11-20 [email protected] Roll Flutter Engine from 3245c8976976 to 3828681d1f86 (1 revision) (flutter/flutter#159217) 2024-11-20 [email protected] Add `--dry-run` to `dev/bots/test.dart`. (flutter/flutter#158956) 2024-11-20 [email protected] Add docs for setting up Android Studio to auto format Kotlin code (flutter/flutter#159209) 2024-11-20 [email protected] Roll Flutter Engine from 80d77505fdde to 3245c8976976 (1 revision) (flutter/flutter#159214) 2024-11-20 [email protected] Fix: The enableFeedback property of InkWell cannot be set to a nullabâ�¦ (flutter/flutter#158907) 2024-11-20 [email protected] Roll Flutter Engine from 3f19207e820e to 80d77505fdde (1 revision) (flutter/flutter#159210) 2024-11-20 [email protected] Roll Packages from fc4adc7 to e95f6d8 (6 revisions) (flutter/flutter#159201) 2024-11-20 [email protected] Remove dependency on [Target] and instead operate on [Architecture] (flutter/flutter#159196) 2024-11-20 [email protected] Fix git command in Quality-Assurance.md (flutter/flutter#155146) 2024-11-20 [email protected] Roll Flutter Engine from 7eb87547cbc6 to 3f19207e820e (4 revisions) (flutter/flutter#159176) 2024-11-20 [email protected] Make `runner` non-nullable as it always is. (flutter/flutter#159156) 2024-11-19 [email protected] Update Material 3 `CircularProgressIndicator` for new visual style (flutter/flutter#158104) 2024-11-19 [email protected] Roll Flutter Engine from 4ff696b555dc to 7eb87547cbc6 (3 revisions) (flutter/flutter#159168) 2024-11-19 [email protected] Add platform-android label for all flutter_tools *android* files (flutter/flutter#159166) 2024-11-19 [email protected] Roll Flutter Engine from d5820a638885 to 4ff696b555dc (1 revision) (flutter/flutter#159164) 2024-11-19 [email protected] Reland Add UI Benchmarks (flutter/flutter#153368) 2024-11-19 [email protected] fix lint usage of `task` inside `resolve_dependecies.gradle` file (flutter/flutter#158022) 2024-11-19 [email protected] Roll Flutter Engine from cff1e751f853 to d5820a638885 (5 revisions) (flutter/flutter#159155) 2024-11-19 [email protected] Removing redundant backticks in `flutter\packages\flutter_tools\gradle\gradle.kts` (flutter/flutter#159051) 2024-11-19 [email protected] Fixes initial validation with AutovalidateMode.always on first build (flutter/flutter#156708) 2024-11-19 [email protected] Introduce new Material 3 `Slider` shapes (flutter/flutter#152237) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose ...

…on (flutter/flutter#159170)

matanlurey added 2 commits November 19, 2024 12:08

Terminate the test device unless detach is explicitly used.

51d8c86

++

6eb93ec

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 19, 2024

Write tests.

3654b29

matanlurey changed the title ~~WIP: Terminate non-detached test devices on flutter run completion~~ Terminate non-detached test devices on flutter run completion Nov 19, 2024

matanlurey requested review from andrewkolos and jason-simmons November 19, 2024 22:45

matanlurey marked this pull request as ready for review November 19, 2024 22:45

andrewkolos reviewed Nov 19, 2024

View reviewed changes

packages/flutter_tools/test/general.shard/run_hot_test.dart Show resolved Hide resolved

andrewkolos reviewed Nov 19, 2024

View reviewed changes

packages/flutter_tools/test/general.shard/run_hot_test.dart Outdated Show resolved Hide resolved

andrewkolos reviewed Nov 19, 2024

View reviewed changes

packages/flutter_tools/test/general.shard/run_hot_test.dart Show resolved Hide resolved

andrewkolos reviewed Nov 19, 2024

View reviewed changes

packages/flutter_tools/test/general.shard/run_hot_test.dart Show resolved Hide resolved

andrewkolos previously approved these changes Nov 19, 2024

View reviewed changes

jason-simmons previously requested changes Nov 19, 2024

View reviewed changes

matanlurey marked this pull request as draft November 20, 2024 18:11

Address feedback.

f55a203

matanlurey requested review from andrewkolos and jason-simmons November 20, 2024 19:32

matanlurey marked this pull request as ready for review November 20, 2024 19:32

matanlurey mentioned this pull request Nov 20, 2024

flutter_tester process is not terminated when a test run is terminated #144222

Closed

Update errenous printHelp.

7a5fc4a

jason-simmons reviewed Nov 20, 2024

View reviewed changes

Rename.

5e6354f

matanlurey requested a review from jason-simmons November 20, 2024 21:37

auto-submit bot added this pull request to the merge queue Nov 21, 2024

Merged via the queue into flutter:master with commit 5ead4e1 Nov 21, 2024
145 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2024

engine-flutter-autoroll mentioned this pull request Nov 21, 2024

Roll Flutter from 8536b96ebb3e to 1578d7d5f34e (28 revisions) flutter/packages#8139

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2024

5ead4e1 Terminate non-detached test devices on flutter run completi…

f073911

…on (flutter/flutter#159170)

engine-flutter-autoroll mentioned this pull request Nov 21, 2024

Roll Flutter from 8536b96ebb3e to 7662e7eae548 (29 revisions) flutter/packages#8140

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2024

5ead4e1 Terminate non-detached test devices on flutter run completi…

107afe7

…on (flutter/flutter#159170)

engine-flutter-autoroll mentioned this pull request Nov 21, 2024

Roll Flutter from 8536b96ebb3e to 81d2bf5d7934 (30 revisions) flutter/packages#8141

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2024

5ead4e1 Terminate non-detached test devices on flutter run completi…

89ed193

…on (flutter/flutter#159170)

engine-flutter-autoroll mentioned this pull request Nov 21, 2024

Roll Flutter from 8536b96ebb3e to 4e03220a8691 (31 revisions) flutter/packages#8143

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2024

5ead4e1 Terminate non-detached test devices on flutter run completi…

c1d9d4a

…on (flutter/flutter#159170)

engine-flutter-autoroll mentioned this pull request Nov 21, 2024

Roll Flutter from 8536b96ebb3e to a051be858576 (32 revisions) flutter/packages#8144

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2024

5ead4e1 Terminate non-detached test devices on flutter run completi…

80e915c

…on (flutter/flutter#159170)

engine-flutter-autoroll mentioned this pull request Nov 21, 2024

Roll Flutter from 8536b96ebb3e to d1e2c5fba748 (33 revisions) flutter/packages#8145

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2024

5ead4e1 Terminate non-detached test devices on flutter run completi…

03a0540

…on (flutter/flutter#159170)

engine-flutter-autoroll mentioned this pull request Nov 21, 2024

Roll Flutter from 8536b96ebb3e to 399403ae6a38 (36 revisions) flutter/packages#8146

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2024

5ead4e1 Terminate non-detached test devices on flutter run completi…

b33114f

…on (flutter/flutter#159170)

engine-flutter-autoroll mentioned this pull request Nov 21, 2024

Roll Flutter from 8536b96ebb3e to 93d772c5cdd8 (37 revisions) flutter/packages#8147

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2024

5ead4e1 Terminate non-detached test devices on flutter run completi…

2707bbb

…on (flutter/flutter#159170)

reidbaker mentioned this pull request Dec 13, 2024

Cutout rotation integration test #160255

Closed

11 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025

5ead4e1 Terminate non-detached test devices on flutter run completi…

ef1c946

…on (flutter/flutter#159170)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

5ead4e1 Terminate non-detached test devices on flutter run completi…

075ae94

…on (flutter/flutter#159170)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

5ead4e1 Terminate non-detached test devices on flutter run completi…

9da6489

…on (flutter/flutter#159170)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025

5ead4e1 Terminate non-detached test devices on flutter run completi…

c6c4b8d

…on (flutter/flutter#159170)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025

5ead4e1 Terminate non-detached test devices on flutter run completi…

b26bc04

…on (flutter/flutter#159170)

This was referenced Jul 22, 2025

Sending q (quit) when in flutter attach doesn't kill the running app #122303

Closed

flutter attach commands are misleading #119550

Closed

Terminate non-detached test devices on flutter run completion #159170

Terminate non-detached test devices on flutter run completion #159170

Uh oh!

Conversation

matanlurey commented Nov 19, 2024 • edited by jason-simmons Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewkolos left a comment

Choose a reason for hiding this comment

Uh oh!

jason-simmons left a comment

Choose a reason for hiding this comment

Uh oh!

jason-simmons commented Nov 20, 2024

Uh oh!

andrewkolos commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewkolos commented Nov 20, 2024

Uh oh!

matanlurey commented Nov 20, 2024

Uh oh!

matanlurey commented Nov 20, 2024

Uh oh!

jason-simmons left a comment

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Terminate non-detached test devices on `flutter run` completion #159170

Terminate non-detached test devices on `flutter run` completion #159170

matanlurey commented Nov 19, 2024 •

edited by jason-simmons

Loading

andrewkolos commented Nov 20, 2024 •

edited

Loading