when setting up the log reader for a device during `flutter run`, discard any `RPCError` thrown due to the device being disconnected #155049

andrewkolos · 2024-09-11T21:25:52Z

This PR contains some refactoring. To make the actual change easier to figure out, I've tried to separate parts of the change into multiple commits for easier reviewing 🙂.

I plan on cherry-picking this change to stable.

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.

This is to hint that this will be a no-op if the device is offline.

andrewkolos · 2024-09-11T22:22:27Z

packages/flutter_tools/lib/src/vmservice.dart

+      if (err.code == RPCErrorCodes.kServiceDisappeared ||
+          err.message.contains('Service connection disposed')) {


I'm aware of dart-lang/sdk@e099a00, but I don't think the new error code is exposed anywhere yet.

Yeah, it looks like we'll need to publish a new version of package:vm_service, but if we're going to be CP'ing this change this is probably sufficient for now.

bkonyi · 2024-09-12T15:38:29Z

packages/flutter_tools/lib/src/vmservice.dart

+      if (err.code == RPCErrorCodes.kServiceDisappeared ||
+          err.message.contains('Service connection disposed')) {


Yeah, it looks like we'll need to publish a new version of package:vm_service, but if we're going to be CP'ing this change this is probably sufficient for now.

flutteractionsbot · 2024-09-12T15:43:19Z

Failed to create CP due to merge conflicts.
You will need to create the PR manually. See the cherrypick wiki for more info.

…un`, discard any `RPCError` thrown due to the device being disconnected (flutter/flutter#155049)

…card any `RPCError` thrown due to the device being disconnected (flutter#155049) Fixes flutter#154903 This PR contains some refactoring. To make the actual change easier to figure out, I've tried to separate parts of the change into multiple commits for easier reviewing ð��. **I plan on cherry-picking this change to stable.**

andrewkolos · 2024-09-12T16:37:28Z

Holding off on the cherry-pick.

I chatted with @jonahwilliams and did some quick experiments and figured out FlutterDevice.tryInitLogReader (formerly initLogReader) doesn't do what I thought it does. All it does is set the appPid field on the device's DeviceLogReader. The only reference of this I could find of appPid is

flutter/packages/flutter_tools/lib/src/android/android_device.dart

Lines 1192 to 1199 in 7292c94

    
           } else if (appPid != null && int.parse(logMatch.group(1)!) == appPid) { 
        
             acceptLine = !_surfaceSyncerSpam.hasMatch(line); 
        
             if (_fatalLog.hasMatch(line)) { 
        
               // Hit fatal signal, app is now crashing 
        
               _fatalCrash = true; 
        
             } 
        
           } else {

For context (though I'm no expert here): To get logs from Android, we need to use adb logcat; but gives us more than just the logs from the Flutter app. That's why we need this extra logic to filter out all of the noise.

Why this matters. For users not running apps on Android, the error message I introduced would be pointless and confusing noise.

I'll see if there is a way to relocate this tryInitLogReader away from the general ResidentRunner workflow and over to the Android side of things. If that fails, I can at least update the error message to be less confusing (as-is, the message implies that logging won't work, but in reality it still will—though the Android workflow might become unstable if the device is running other apps...hmm, maybe we should throwToolExit in that case...).

…un`, discard any `RPCError` thrown due to the device being disconnected (flutter/flutter#155049)

Roll Flutter from 2e221e7 to 303f222 (77 revisions) flutter/flutter@2e221e7...303f222 2024-09-12 [email protected] Manual roll to 48ddaf578fb0c8326d5b4b680b0f49ea72e33216 (flutter/flutter#155070) 2024-09-12 [email protected] Externalize and update onboarding instructions (flutter/flutter#154730) 2024-09-12 [email protected] when setting up the log reader for a device during `flutter run`, discard any `RPCError` thrown due to the device being disconnected (flutter/flutter#155049) 2024-09-12 98614782+auto-submit[bot]@users.noreply.github.com Reverts "iOS: update provisioning profile for 2024-2025 cert (#155052)" (flutter/flutter#155059) 2024-09-12 [email protected] iOS: update provisioning profile for 2024-2025 cert (flutter/flutter#155052) 2024-09-11 [email protected] Factor out `Container` objects (flutter/flutter#153619) 2024-09-11 [email protected] Move (`dev/tools`), complete v0 of `native_driver` (Android) (flutter/flutter#154843) 2024-09-11 [email protected] Roll Flutter Engine from ade8ef293bc6 to ee5adf6d2ee1 (2 revisions) (flutter/flutter#155046) 2024-09-11 [email protected] Fix `flutter run` on Mac x64 hosts if Swift Package Manager is enabled (flutter/flutter#154645) 2024-09-11 [email protected] Roll Packages from bb53e5d to 4c18648 (1 revision) (flutter/flutter#155033) 2024-09-11 [email protected] Roll Flutter Engine from 4eb729b7a5c4 to ade8ef293bc6 (3 revisions) (flutter/flutter#155031) 2024-09-11 [email protected] fix: Dropdown menu trying to access highlight element which doesn't exist when search and filters both are enabled (flutter/flutter#151969) 2024-09-11 [email protected] Marks Linux build_tests_3_5 to be unflaky (flutter/flutter#154993) 2024-09-11 [email protected] Add 'direction' allow to 'SegmentedButton' oriented vertically (flutter/flutter#150903) 2024-09-11 [email protected] Marks Linux build_tests_5_5 to be unflaky (flutter/flutter#154995) 2024-09-11 [email protected] Update the signature of DDS launcher callback. (flutter/flutter#154949) 2024-09-11 [email protected] Migrate Color.toString() test, improves `equalsIgnoringHashCodes` (flutter/flutter#154934) 2024-09-11 [email protected] Update material and cupertino localizations (flutter/flutter#154959) 2024-09-11 [email protected] Marks Linux build_tests_1_5 to be unflaky (flutter/flutter#154991) 2024-09-11 [email protected] Marks Linux build_tests_2_5 to be unflaky (flutter/flutter#154992) 2024-09-11 [email protected] Fix `flutter create` warning regarding Java compatibility (flutter/flutter#152836) 2024-09-11 [email protected] Roll Flutter Engine from 54757dab9462 to 4eb729b7a5c4 (1 revision) (flutter/flutter#155022) 2024-09-11 [email protected] Fix java version used by `build_aar_module_test` (flutter/flutter#154967) 2024-09-11 [email protected] Roll Flutter Engine from 0a14c519ea4f to 54757dab9462 (1 revision) (flutter/flutter#155015) 2024-09-11 [email protected] Roll Flutter Engine from 35a3171b72c5 to 0a14c519ea4f (1 revision) (flutter/flutter#154984) 2024-09-11 [email protected] Roll Flutter Engine from b9c0b96c3316 to 35a3171b72c5 (1 revision) (flutter/flutter#154980) 2024-09-11 [email protected] Roll Flutter Engine from 52eeea075767 to b9c0b96c3316 (1 revision) (flutter/flutter#154976) 2024-09-11 [email protected] Roll Flutter Engine from a26075f9b1e6 to 52eeea075767 (1 revision) (flutter/flutter#154973) 2024-09-11 [email protected] Roll Flutter Engine from 60c15bc0f40e to a26075f9b1e6 (6 revisions) (flutter/flutter#154969) 2024-09-11 [email protected] Migrate `apple-mobile-web-*` to `mobile-web-*`. (flutter/flutter#154964) 2024-09-11 [email protected] Roll Flutter Engine from 8a038a6f7099 to 60c15bc0f40e (15 revisions) (flutter/flutter#154960) 2024-09-10 [email protected] Adds dart fixes for Color opacity functions (flutter/flutter#154953) 2024-09-10 [email protected] Missing benchmarks for `foundation/all_elements_bench.dart` (flutter/flutter#154954) 2024-09-10 [email protected] Update color assertions (flutter/flutter#154752) 2024-09-10 [email protected] handle EAGAIN (macOS) in ErrorHandlingProcessManager (flutter/flutter#154306) 2024-09-10 [email protected] fix unpack freezing app with animation duration zero (flutter/flutter#153890) 2024-09-10 [email protected] Remove last `--disable-dart-dev` in `flutter/flutter`. (flutter/flutter#154948) 2024-09-10 [email protected] Remove scheduler: luci from new `build_aar_module_test` (flutter/flutter#154945) 2024-09-10 [email protected] Roll pub packages (flutter/flutter#154939) 2024-09-10 [email protected] `CupertinoSlidingSegmentedControl` update (flutter/flutter#152976) 2024-09-10 [email protected] Roll pub packages (flutter/flutter#154933) 2024-09-10 [email protected] fix test `chrome.close can recover if getTab throws a StateError` (flutter/flutter#154889) 2024-09-10 [email protected] SearchBar context menu (flutter/flutter#154833) 2024-09-10 [email protected] Fix `flutter build aar` for modules that use a plugin (flutter/flutter#154757) 2024-09-10 [email protected] Roll Packages from b4e0fc1 to bb53e5d (4 revisions) (flutter/flutter#154926) 2024-09-10 [email protected] Clean up `SnackBar` inherit theme data test (flutter/flutter#154921) ...

…roid (#155800) This is a follow-up to the PR #155049 (which fixed #154903). This PR addresses the resulting issue, #155795. It does so in a hacky way for the sake of simplicity (and thus suitability for cherry-picking). I intend to clean this up on the master channel with yet another follow-up PR, #155796, which currently exists as a proof-of-concept to make sure I actually have the ability to clean this after myself here. **I intend to submit a stable hotfix patch with the changes from the original fix (#154903) and the follow-up changes from this PR.**

…card any `RPCError` thrown due to the device being disconnected (flutter#155049) Fixes flutter#154903 This PR contains some refactoring. To make the actual change easier to figure out, I've tried to separate parts of the change into multiple commits for easier reviewing ð��. **I plan on cherry-picking this change to stable.**

…roid (flutter#155800) This is a follow-up to the PR flutter#155049 (which fixed flutter#154903). This PR addresses the resulting issue, flutter#155795. It does so in a hacky way for the sake of simplicity (and thus suitability for cherry-picking). I intend to clean this up on the master channel with yet another follow-up PR, flutter#155796, which currently exists as a proof-of-concept to make sure I actually have the ability to clean this after myself here. **I intend to submit a stable hotfix patch with the changes from the original fix (flutter#154903) and the follow-up changes from this PR.**

…un`, discard any `RPCError` thrown due to the device being disconnected (flutter/flutter#155049)

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

andrewkolos force-pushed the guard-getVm-calls branch from 7220741 to 12e5190 Compare September 11, 2024 21:37

andrewkolos changed the title ~~catch RPCError due to disconnection when initiating device log reader~~ when setting up the log reader for a device during flutter run, discard any RPCError thrown due to the device being disconnected Sep 11, 2024

andrewkolos added 4 commits September 11, 2024 15:19

add test

2a7693a

introduce FlutterVmService.getVmGuarded

f5ad26d

make FlutterDevice.initLogReader handle VM service disconnection

18572a7

refactor: rename initLogReader to tryInitLogReader

30f42a0

This is to hint that this will be a no-op if the device is offline.

andrewkolos force-pushed the guard-getVm-calls branch from c9c5ba1 to 30f42a0 Compare September 11, 2024 22:20

andrewkolos commented Sep 11, 2024

View reviewed changes

andrewkolos marked this pull request as ready for review September 12, 2024 01:30

andrewkolos requested a review from bkonyi September 12, 2024 01:34

bkonyi approved these changes Sep 12, 2024

View reviewed changes

andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 12, 2024

auto-submit bot merged commit d8f8613 into flutter:master Sep 12, 2024

andrewkolos added the cp: stable cherry pick this pull request to stable release candidate branch label Sep 12, 2024

engine-flutter-autoroll mentioned this pull request Sep 12, 2024

Roll Flutter from 2e221e7308ba to d8f8613a5b94 (75 revisions) flutter/packages#7635

Closed

engine-flutter-autoroll mentioned this pull request Sep 12, 2024

Roll Flutter from 2e221e7308ba to 90aa59f6c32f (76 revisions) flutter/packages#7637

Closed

engine-flutter-autoroll mentioned this pull request Sep 12, 2024

Roll Flutter from 2e221e7308ba to 303f222e17e3 (77 revisions) flutter/packages#7638

Merged

andrewkolos mentioned this pull request Oct 1, 2024

[CP-stable] when filtering logs from adb by process ID fails, log warning (if targeting Android) instead of crashing #155955

Closed

9 tasks

andrewkolos mentioned this pull request Oct 7, 2024

handle any RPCError due to vm service disconnection in flutter run #156346

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

when setting up the log reader for a device during `flutter run`, discard any `RPCError` thrown due to the device being disconnected #155049

when setting up the log reader for a device during `flutter run`, discard any `RPCError` thrown due to the device being disconnected #155049

Uh oh!

andrewkolos commented Sep 11, 2024 •

edited

Loading

Uh oh!

andrewkolos Sep 11, 2024

Uh oh!

bkonyi Sep 12, 2024

Uh oh!

bkonyi Sep 12, 2024

Uh oh!

flutteractionsbot commented Sep 12, 2024

Uh oh!

andrewkolos commented Sep 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		if (err.code == RPCErrorCodes.kServiceDisappeared \|\|
		err.message.contains('Service connection disposed')) {

when setting up the log reader for a device during flutter run, discard any RPCError thrown due to the device being disconnected #155049

when setting up the log reader for a device during flutter run, discard any RPCError thrown due to the device being disconnected #155049

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

andrewkolos Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

bkonyi Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

bkonyi Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

flutteractionsbot commented Sep 12, 2024

Uh oh!

andrewkolos commented Sep 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

when setting up the log reader for a device during `flutter run`, discard any `RPCError` thrown due to the device being disconnected #155049

when setting up the log reader for a device during `flutter run`, discard any `RPCError` thrown due to the device being disconnected #155049

andrewkolos commented Sep 11, 2024 •

edited

Loading