[iOS] Dispose of log readers and port forwarders if launch fails #127140

vashworth · 2023-05-18T21:24:49Z

When tests run in our CI using flutter drive, if there is a failure it will loop and try again.

flutter/packages/flutter_tools/lib/src/drive/drive_service.dart

Lines 177 to 186 in 434b81f

    
           while (attempt < _kLaunchAttempts) { 
        
             result = await device.startApp( 
        
               _applicationPackage, 
        
               mainPath: mainPath, 
        
               route: route, 
        
               debuggingOptions: debuggingOptions, 
        
               platformArgs: platformArgs, 
        
               userIdentifier: userIdentifier, 
        
               prebuiltApplication: prebuiltApplication, 
        
             );

However, it's using the same device instance for each iteration. So what was happening was when it would fail to launch, it would tell its listeners that it was cancelled.

flutter/packages/flutter_tools/lib/src/ios/ios_deploy.dart

Lines 486 to 489 in 434b81f

    
           if (_debuggerOutput.hasListener) { 
        
             // Tell listeners the process died. 
        
             await _debuggerOutput.close(); 
        
           }

Then when the next iteration started, the vmServiceDiscovery would immediately return with null because the deviceLogReader would be cached from the previous iteration and would already be cancelled. Therefore, bypassing and cancelling the timer.

flutter/packages/flutter_tools/lib/src/ios/devices.dart

Lines 585 to 591 in 434b81f

    
             localUri = await vmServiceDiscovery?.uri; 
        
           } 
        
           timer.cancel(); 
        
           if (localUri == null) { 
        
             await iosDeployDebugger?.stopAndDumpBacktrace(); 
        
             return LaunchResult.failed(); 
        
           }

flutter/packages/flutter_tools/lib/src/ios/devices.dart

Line 627 in 434b81f

return _logReaders.putIfAbsent(app, () => IOSDeviceLogReader.create(

In addition, it seems like sometimes the stop would fail and therefore the the drain would never get the signal that it was done and therefore would hang forever. There was no indication that the stop had failed though because the logs were going to the stream that had no listeners since deviceLogReader was already cancelled.

flutter/packages/flutter_tools/lib/src/ios/ios_deploy.dart

Lines 563 to 576 in 434b81f

    
           Future<void> stopAndDumpBacktrace() async { 
        
             if (!debuggerAttached) { 
        
               return; 
        
             } 
        
             try { 
        
               // Stop the app, which will prompt the backtrace to be printed for all threads in the stdoutSubscription handler. 
        
               _iosDeployProcess?.stdin.writeln(_signalStop); 
        
             } on SocketException catch (error) { 
        
               // Best effort, try to detach, but maybe the app already exited or already detached. 
        
               _logger.printTrace('Could not stop app from debugger: $error'); 
        
             } 
        
             // Wait for logging to finish on process exit. 
        
             return logLines.drain(); 
        
           }

Fixes #127141

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.
All existing and new tests are passing.

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

christopherfujino · 2023-05-18T22:12:08Z

packages/flutter_tools/lib/src/ios/devices.dart

      _logger.printError(e.message);
+      await dispose();
      return LaunchResult.failed();
    } finally {


any reason not to just call dispose() from the finally block?

We don't want it to dispose when it launches successfully

ahh yeah, good point :)

christopherfujino · 2023-05-18T22:13:16Z

packages/flutter_tools/test/general.shard/ios/ios_deploy_test.dart

+        final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
+          FakeCommand(
+            command: const <String>['ios-deploy'],
+            stdout: '(lldb)     run\r\nsuccess\r\nsuccess\r\nprocess signal SIGSTOP\r\n\r\nerror: Failed to send signal 17: failed to send signal 17',


Out of curiosity, would we actually expect to see '\r' here?

Oh good question, I'm not sure. I just grabbed that from the test above. I'll try and figure out

not worth investigating, just wondering if there was a legit reason.

I just happened to come across the answer to this:

flutter/packages/flutter_tools/lib/src/ios/ios_deploy.dart

Lines 477 to 480 in 4f6f188

// The lldb console stream from ios-deploy is separated lines by an extra \r\n.

// To avoid all lines being double spaced, if the last line from the

// debugger was not an empty line, skip this empty line.

// This will still cause "legit" logged newlines to be doubled...

christopherfujino · 2023-05-18T22:21:16Z

packages/flutter_tools/test/general.shard/ios/ios_deploy_test.dart

        expect(logger.traceText, contains('* thread #1'));
      });

+      testWithoutContext('debugger attached and stop failed', () async {


Is this test verifying any of your changes here? It's passing for me if I revert your other changes.

Oops, let me rework it

Okay updated it

christopherfujino

Nice! Thanks for this fix!

…ils (flutter/flutter#127140)

flutter/flutter@077d644...ab57304 2023-05-20 [email protected] Roll Flutter Engine from 482c99af9c69 to aac09195688d (1 revision) (flutter/flutter#127241) 2023-05-20 [email protected] Reland "[tool] Move Java functions to their own file" (flutter/flutter#126577) 2023-05-20 [email protected] Roll Flutter Engine from f0c02aee69db to 482c99af9c69 (1 revision) (flutter/flutter#127240) 2023-05-19 [email protected] Do not animate `TabBarView` if controller is invalid (flutter/flutter#123442) 2023-05-19 [email protected] Run Mac intel only targets on both intel and arm (flutter/flutter#127230) 2023-05-19 [email protected] [Windows] Ensure window is shown (flutter/flutter#127046) 2023-05-19 [email protected] Roll Flutter Engine from 3267fa29491a to f0c02aee69db (4 revisions) (flutter/flutter#127233) 2023-05-19 [email protected] Roll Flutter Engine from 2b14f8a1f21c to 3267fa29491a (4 revisions) (flutter/flutter#127224) 2023-05-19 [email protected] fixes to anticipate next Dart linter release (flutter/flutter#127211) 2023-05-19 [email protected] Remove deprecated OverscrollIndicatorNotification.disallowGlow (flutter/flutter#127050) 2023-05-19 [email protected] Roll Flutter Engine from f471b37a2146 to 2b14f8a1f21c (1 revision) (flutter/flutter#127221) 2023-05-19 [email protected] [flutter_tools] only try to take a screenshot from flutter drive if the --screenshot flag is passed (flutter/flutter#127150) 2023-05-19 [email protected] Roll goldctl to f808dcff91b221ae313e540c09d79696cd08b8de (flutter/flutter#127218) 2023-05-19 [email protected] Roll Packages from b31a128 to 1e214d7 (3 revisions) (flutter/flutter#127217) 2023-05-19 [email protected] Roll Flutter Engine from a0ea4d2d9ea5 to f471b37a2146 (1 revision) (flutter/flutter#127212) 2023-05-19 [email protected] Revert "Migrate benchmarks to package:web" (flutter/flutter#127207) 2023-05-19 [email protected] [tool] delete xcresult bundle file before each xcode retry. (flutter/flutter#127144) 2023-05-19 [email protected] [iOS] Dispose of log readers and port forwarders if launch fails (flutter/flutter#127140) 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],[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 To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…tter#127140) When tests run in our CI using `flutter drive`, if there is a failure it will loop and try again. https://github.com/flutter/flutter/blob/434b81f1a58b8528299a63ae7c3ded2aa519e3c9/packages/flutter_tools/lib/src/drive/drive_service.dart#L177-L186 However, it's using the same `device` instance for each iteration. So what was happening was when it would fail to launch, it would tell its listeners that it was cancelled. https://github.com/flutter/flutter/blob/434b81f1a58b8528299a63ae7c3ded2aa519e3c9/packages/flutter_tools/lib/src/ios/ios_deploy.dart#L486-L489 Then when the next iteration started, the `vmServiceDiscovery` would immediately return with null because the `deviceLogReader` would be cached from the previous iteration and would already be cancelled. Therefore, bypassing and cancelling the timer. https://github.com/flutter/flutter/blob/434b81f1a58b8528299a63ae7c3ded2aa519e3c9/packages/flutter_tools/lib/src/ios/devices.dart#L585-L591 https://github.com/flutter/flutter/blob/434b81f1a58b8528299a63ae7c3ded2aa519e3c9/packages/flutter_tools/lib/src/ios/devices.dart#L627 In addition, it seems like sometimes the stop would fail and therefore the the drain would never get the signal that it was done and therefore would hang forever. There was no indication that the stop had failed though because the logs were going to the stream that had no listeners since `deviceLogReader` was already cancelled. https://github.com/flutter/flutter/blob/434b81f1a58b8528299a63ae7c3ded2aa519e3c9/packages/flutter_tools/lib/src/ios/ios_deploy.dart#L563-L576 Fixes flutter#127141

flutter/flutter@077d644...ab57304 2023-05-20 [email protected] Roll Flutter Engine from 482c99af9c69 to aac09195688d (1 revision) (flutter/flutter#127241) 2023-05-20 [email protected] Reland "[tool] Move Java functions to their own file" (flutter/flutter#126577) 2023-05-20 [email protected] Roll Flutter Engine from f0c02aee69db to 482c99af9c69 (1 revision) (flutter/flutter#127240) 2023-05-19 [email protected] Do not animate `TabBarView` if controller is invalid (flutter/flutter#123442) 2023-05-19 [email protected] Run Mac intel only targets on both intel and arm (flutter/flutter#127230) 2023-05-19 [email protected] [Windows] Ensure window is shown (flutter/flutter#127046) 2023-05-19 [email protected] Roll Flutter Engine from 3267fa29491a to f0c02aee69db (4 revisions) (flutter/flutter#127233) 2023-05-19 [email protected] Roll Flutter Engine from 2b14f8a1f21c to 3267fa29491a (4 revisions) (flutter/flutter#127224) 2023-05-19 [email protected] fixes to anticipate next Dart linter release (flutter/flutter#127211) 2023-05-19 [email protected] Remove deprecated OverscrollIndicatorNotification.disallowGlow (flutter/flutter#127050) 2023-05-19 [email protected] Roll Flutter Engine from f471b37a2146 to 2b14f8a1f21c (1 revision) (flutter/flutter#127221) 2023-05-19 [email protected] [flutter_tools] only try to take a screenshot from flutter drive if the --screenshot flag is passed (flutter/flutter#127150) 2023-05-19 [email protected] Roll goldctl to f808dcff91b221ae313e540c09d79696cd08b8de (flutter/flutter#127218) 2023-05-19 [email protected] Roll Packages from b31a128 to 1e214d7 (3 revisions) (flutter/flutter#127217) 2023-05-19 [email protected] Roll Flutter Engine from a0ea4d2d9ea5 to f471b37a2146 (1 revision) (flutter/flutter#127212) 2023-05-19 [email protected] Revert "Migrate benchmarks to package:web" (flutter/flutter#127207) 2023-05-19 [email protected] [tool] delete xcresult bundle file before each xcode retry. (flutter/flutter#127144) 2023-05-19 [email protected] [iOS] Dispose of log readers and port forwarders if launch fails (flutter/flutter#127140) 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],[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 To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ils (flutter/flutter#127140)

Dispose of log readers and port forwarders if launch fails

110f481

flutter-dashboard bot added platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. labels May 18, 2023

add handling for stop failure

bba805e

vashworth marked this pull request as ready for review May 18, 2023 22:09

vashworth requested a review from christopherfujino May 18, 2023 22:09

Merge branch 'master' into timeout-fix-1

41813ba

christopherfujino reviewed May 18, 2023

View reviewed changes

fix test

152b393

christopherfujino approved these changes May 18, 2023

View reviewed changes

vashworth added the autosubmit Merge PR when tree becomes green via auto submit App label May 19, 2023

auto-submit bot merged commit b9e8b0a into flutter:master May 19, 2023

vashworth mentioned this pull request May 19, 2023

Mac_ios hot_mode_dev_cycle_ios__benchmark is 2.00% flaky, Process exited with status = -1 (0xffffffff) lost connection #120808

Closed

engine-flutter-autoroll mentioned this pull request May 20, 2023

Roll Flutter from 077d644bbc5f to 80a4f9b15917 (17 revisions) flutter/packages#4047

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2023

b9e8b0a [iOS] Dispose of log readers and port forwarders if launch fa…

4bed3d3

…ils (flutter/flutter#127140)

engine-flutter-autoroll mentioned this pull request May 20, 2023

Roll Flutter from 077d644bbc5f to ab573048e74d (18 revisions) flutter/packages#4048

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2023

b9e8b0a [iOS] Dispose of log readers and port forwarders if launch fa…

2f4a566

…ils (flutter/flutter#127140)

engine-flutter-autoroll mentioned this pull request May 21, 2023

Roll Flutter from 077d644bbc5f to ab573048e74d (18 revisions) flutter/packages#4050

Closed

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

b9e8b0a [iOS] Dispose of log readers and port forwarders if launch fa…

87abf56

…ils (flutter/flutter#127140)

engine-flutter-autoroll mentioned this pull request May 21, 2023

Roll Flutter from 077d644bbc5f to ab573048e74d (18 revisions) flutter/packages#4051

Merged

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

b9e8b0a [iOS] Dispose of log readers and port forwarders if launch fa…

1831fcb

…ils (flutter/flutter#127140)

This was referenced May 25, 2023

Support minifcation for apps that depend on AGP 8 and integration_test #127628

Merged

i127682 android api 33 test support #127689

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

b9e8b0a [iOS] Dispose of log readers and port forwarders if launch fa…

007b4a3

…ils (flutter/flutter#127140)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

b9e8b0a [iOS] Dispose of log readers and port forwarders if launch fa…

e24fcae

…ils (flutter/flutter#127140)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

b9e8b0a [iOS] Dispose of log readers and port forwarders if launch fa…

43e2219

…ils (flutter/flutter#127140)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

b9e8b0a [iOS] Dispose of log readers and port forwarders if launch fa…

ea011bf

…ils (flutter/flutter#127140)

vashworth deleted the timeout-fix-1 branch September 27, 2023 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[iOS] Dispose of log readers and port forwarders if launch fails #127140

[iOS] Dispose of log readers and port forwarders if launch fails #127140

Uh oh!

vashworth commented May 18, 2023 •

edited

Loading

Uh oh!

christopherfujino May 18, 2023

Uh oh!

vashworth May 18, 2023

Uh oh!

christopherfujino May 18, 2023

Uh oh!

christopherfujino May 18, 2023

Uh oh!

vashworth May 18, 2023

Uh oh!

christopherfujino May 18, 2023

Uh oh!

vashworth May 19, 2023

Uh oh!

christopherfujino May 18, 2023

Uh oh!

vashworth May 18, 2023

Uh oh!

vashworth May 18, 2023

Uh oh!

christopherfujino left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	while (attempt < _kLaunchAttempts) {
	result = await device.startApp(
	_applicationPackage,
	mainPath: mainPath,
	route: route,
	debuggingOptions: debuggingOptions,
	platformArgs: platformArgs,
	userIdentifier: userIdentifier,
	prebuiltApplication: prebuiltApplication,
	);

	if (_debuggerOutput.hasListener) {
	// Tell listeners the process died.
	await _debuggerOutput.close();
	}

	localUri = await vmServiceDiscovery?.uri;
	}
	timer.cancel();
	if (localUri == null) {
	await iosDeployDebugger?.stopAndDumpBacktrace();
	return LaunchResult.failed();
	}

	Future<void> stopAndDumpBacktrace() async {
	if (!debuggerAttached) {
	return;
	}
	try {
	// Stop the app, which will prompt the backtrace to be printed for all threads in the stdoutSubscription handler.
	_iosDeployProcess?.stdin.writeln(_signalStop);
	} on SocketException catch (error) {
	// Best effort, try to detach, but maybe the app already exited or already detached.
	_logger.printTrace('Could not stop app from debugger: $error');
	}
	// Wait for logging to finish on process exit.
	return logLines.drain();
	}

	// The lldb console stream from ios-deploy is separated lines by an extra \r\n.
	// To avoid all lines being double spaced, if the last line from the
	// debugger was not an empty line, skip this empty line.
	// This will still cause "legit" logged newlines to be doubled...

[iOS] Dispose of log readers and port forwarders if launch fails #127140

[iOS] Dispose of log readers and port forwarders if launch fails #127140

Uh oh!

Conversation

vashworth commented May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

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!

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vashworth commented May 18, 2023 •

edited

Loading