Kill interactive script job `xcdevice observe` processes on tool/daemon shutdown #157646

jmagman · 2024-10-25T22:40:44Z

To convince xcdevice observe to redirect to stdout it's being launched in an interactive shell /usr/bin/script -t 0 /dev/null /usr/bin/arch -arm64e xcrun xcdevice observe --usb

flutter/packages/flutter_tools/lib/src/macos/xcdevice.dart

Lines 261 to 263 in 1cc8a07

    
           // Run in interactive mode (via script) to convince 
        
           // xcdevice it has a terminal attached in order to redirect stdout. 
        
           return _streamXCDeviceEventCommand(

When the flutter command exits, the interactive script process is terminated, but not its jobs xcdevice observe --usb.

Instead of -9 sigterm killing the interactive script, send it a SIGHUP (signal hangup) during XCDevice.dispose(), which will exit its processes. Add a shutdown hook to ensure dispose is run when the process exits.

Fixes #73859

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

#157646 is being labeled by the PR labeler as "text-input" because it edited "con**text**_runner.dart". ``` ##[debug] "**/*text*" pattern matched packages/flutter_tools/lib/src/context_runner.dart ``` https://github.com/flutter/flutter/actions/runs/11526508378/job/32090756495?pr=157646#step:2:134 Exclude the common "context" word from this label rule. See negation example at https://github.com/actions/labeler/blob/main/README.md#basic-examples.

…7812) Reverts: #157650 Initiated by: jmagman Reason for reverting: this made the labeler go crazy and put it on everything that didn't contain "context". Original PR Author: jmagman Reviewed By: {christopherfujino} This change reverts the following previous change: #157646 is being labeled by the PR labeler as "text-input" because it edited "con**text**_runner.dart". ``` ##[debug] "**/*text*" pattern matched packages/flutter_tools/lib/src/context_runner.dart ``` https://github.com/flutter/flutter/actions/runs/11526508378/job/32090756495?pr=157646#step:2:134 Exclude the common "context" word from this label rule. See negation example at https://github.com/actions/labeler/blob/main/README.md#basic-examples.

packages/flutter_tools/test/general.shard/macos/xcode_test.dart

andrewkolos · 2024-10-30T21:56:27Z

To convince xcdevice observe to redirect to stdout it's being launched in an interactive shell /usr/bin/script -t 0 /dev/null /usr/bin/arch -arm64e xcrun xcdevice observe --usb

Where does the output usually go if not stdout?

jmagman · 2024-11-05T04:07:17Z

To convince xcdevice observe to redirect to stdout it's being launched in an interactive shell /usr/bin/script -t 0 /dev/null /usr/bin/arch -arm64e xcrun xcdevice observe --usb

Where does the output usually go if not stdout?

If I recall from #58137 (comment) (though it was 4 years ago) it eventually goes to stdout but it waited until the buffer hit some size before writing it, so the output wasn't up-to-date and so we couldn't reliably parse the json since it was output in chunks.

I learned the trick from @cbracken in https://github.com/flutter/flutter/pull/12079/files#diff-5a9a07600dceb28cbe887a5a64a24b0aR502-R504, who filed an issue about a different command with a similar issue https://openradar.appspot.com/34420207

andrewkolos

LGTM

cbracken

…ool/daemon shutdown (flutter/flutter#157646)

flutter/flutter@29d40f7...73546b3 2024-11-06 32538273+ValentinVignal@users.noreply.github.com Add test for `image.loading_builder.0.dart` (flutter/flutter#158248) 2024-11-06 kustermann@google.com Make flutter_tools use newest package:{native_assets_builder,native_assets_cli,native_toolchain_c} (flutter/flutter#158214) 2024-11-06 bruno.leroux@gmail.com Fix RawScrollbar examples and desktop test (flutter/flutter#158237) 2024-11-06 bruno.leroux@gmail.com Cleanup MenuAnchor and Improve DropdownMenu tests readability (flutter/flutter#158175) 2024-11-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from a3741d6248b7 to f03f11300a9d (2 revisions) (flutter/flutter#158222) 2024-11-06 louisehsu@google.com Update error message for Cocoapods support for synchronized groups/folders (flutter/flutter#158206) 2024-11-06 matanlurey@users.noreply.github.com Restore skipped iOS test by looping over `FakeAsync` elapse. (flutter/flutter#158204) 2024-11-06 41366638+kirya355@users.noreply.github.com fix: ensure draggable_scrollable_sheet respects shouldCloseOnMinExtenâ�¦ (flutter/flutter#156338) 2024-11-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from e5e06c97c33c to a3741d6248b7 (14 revisions) (flutter/flutter#158218) 2024-11-06 matanlurey@users.noreply.github.com Forward fix `CupertinoDynamicColor` by adding `toARGB32()`. (flutter/flutter#158145) 2024-11-05 matanlurey@users.noreply.github.com Remove unused `enableObservatory` flag. (flutter/flutter#158202) 2024-11-05 matanlurey@users.noreply.github.com Remove observatory related TODO that is already fixed. (flutter/flutter#158205) 2024-11-05 nate.w5687@gmail.com Factor out "shaker" class (flutter/flutter#157748) 2024-11-05 fluttergithubbot@gmail.com Marks Mac_benchmark animated_complex_opacity_perf_macos__e2e_summary to be flaky (flutter/flutter#157424) 2024-11-05 andrewrkolos@gmail.com Increase subsharding for `Linux tool_integration_tests` (flutter/flutter#158196) 2024-11-05 32538273+ValentinVignal@users.noreply.github.com Add test for `raw_scrollbar.2.dart` (flutter/flutter#158161) 2024-11-05 andrewrkolos@gmail.com use root directory as the default for rootOverride in Cache.test constructor (flutter/flutter#158201) 2024-11-05 magder@google.com Kill interactive script job `xcdevice observe` processes on tool/daemon shutdown (flutter/flutter#157646) 2024-11-05 34465683+rkishan516@users.noreply.github.com Fix: Gap between prefix and suffix icon and input field in input decoâ�¦ (flutter/flutter#152069) 2024-11-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from f56401062e42 to e5e06c97c33c (1 revision) (flutter/flutter#158194) 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 camillesimon@google.com,stuartmorgan@google.com 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 To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ool/daemon shutdown (flutter/flutter#157646)

jmagman self-assigned this Oct 25, 2024

github-actions bot added a: text input Entering text in a text field or keyboard related problems tool Affects the "flutter" command-line tool. See also t: labels. a: desktop Running on desktop labels Oct 25, 2024

jmagman force-pushed the xcdevice-observe-shutdown branch 2 times, most recently from 7371131 to 45b7780 Compare October 25, 2024 22:42

jmagman added platform-ios iOS applications specifically and removed a: text input Entering text in a text field or keyboard related problems a: desktop Running on desktop labels Oct 25, 2024

Kill xcdevice observe processes on tool/daemon shutdown

c0cbb4d

jmagman force-pushed the xcdevice-observe-shutdown branch from 45b7780 to c0cbb4d Compare October 25, 2024 22:56

github-actions bot added a: text input Entering text in a text field or keyboard related problems a: desktop Running on desktop and removed platform-ios iOS applications specifically labels Oct 25, 2024

jmagman mentioned this pull request Oct 25, 2024

"flutter daemon" leaves orphaned "xcdevice observe" processes around after being terminated #73859

Open

jmagman marked this pull request as ready for review October 25, 2024 22:58

jmagman mentioned this pull request Oct 25, 2024

Avoid labeling 'context' PRs as 'text-input' #157650

Merged

9 tasks

jmagman requested a review from cbracken October 26, 2024 02:51

jmagman requested a review from andrewkolos October 28, 2024 21:41

auto-submit bot mentioned this pull request Oct 29, 2024

Reverts "Avoid labeling 'context' PRs as 'text-input' (#157650)" #157812

Merged

9 tasks

jmagman commented Oct 30, 2024

View reviewed changes

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

Remove commented out line

3f09c06

github-actions bot removed the a: text input Entering text in a text field or keyboard related problems label Oct 30, 2024

andrewkolos approved these changes Nov 5, 2024

View reviewed changes

cbracken approved these changes Nov 5, 2024

View reviewed changes

jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 5, 2024

auto-submit bot merged commit b6fef5c into flutter:master Nov 5, 2024

jmagman deleted the xcdevice-observe-shutdown branch November 5, 2024 19:01

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

Roll Flutter from 29d40f7f6826 to 73546b3b71a7 (20 revisions) flutter/packages#8028

Merged

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

b6fef5c Kill interactive script job xcdevice observe processes on t…

879bc07

…ool/daemon shutdown (flutter/flutter#157646)

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

b6fef5c Kill interactive script job xcdevice observe processes on t…

69416be

…ool/daemon shutdown (flutter/flutter#157646)

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

b6fef5c Kill interactive script job xcdevice observe processes on t…

80dfe86

…ool/daemon shutdown (flutter/flutter#157646)

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

b6fef5c Kill interactive script job xcdevice observe processes on t…

fcb7e38

…ool/daemon shutdown (flutter/flutter#157646)

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

b6fef5c Kill interactive script job xcdevice observe processes on t…

00544bf

…ool/daemon shutdown (flutter/flutter#157646)

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

b6fef5c Kill interactive script job xcdevice observe processes on t…

143bfa4

…ool/daemon shutdown (flutter/flutter#157646)

jmagman mentioned this pull request Nov 17, 2025

Use interactive mode with devicectl to redirect stdout #178405

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Kill interactive script job `xcdevice observe` processes on tool/daemon shutdown #157646

Kill interactive script job `xcdevice observe` processes on tool/daemon shutdown #157646

Uh oh!

jmagman commented Oct 25, 2024 •

edited

Loading

Uh oh!

Uh oh!

andrewkolos commented Oct 30, 2024

Uh oh!

jmagman commented Nov 5, 2024

Uh oh!

andrewkolos left a comment

Uh oh!

cbracken left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// Run in interactive mode (via script) to convince
	// xcdevice it has a terminal attached in order to redirect stdout.
	return _streamXCDeviceEventCommand(

Kill interactive script job xcdevice observe processes on tool/daemon shutdown #157646

Kill interactive script job xcdevice observe processes on tool/daemon shutdown #157646

Uh oh!

Conversation

jmagman commented Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

andrewkolos commented Oct 30, 2024

Uh oh!

jmagman commented Nov 5, 2024

Uh oh!

andrewkolos left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kill interactive script job `xcdevice observe` processes on tool/daemon shutdown #157646

Kill interactive script job `xcdevice observe` processes on tool/daemon shutdown #157646

jmagman commented Oct 25, 2024 •

edited

Loading