-
Notifications
You must be signed in to change notification settings - Fork 26.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[iOS] Dispose of log readers and port forwarders if launch fails #127140
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -11,6 +11,7 @@ import 'package:flutter_tools/src/artifacts.dart'; | |||||||||
import 'package:flutter_tools/src/base/file_system.dart'; | ||||||||||
import 'package:flutter_tools/src/base/logger.dart'; | ||||||||||
import 'package:flutter_tools/src/base/platform.dart'; | ||||||||||
import 'package:flutter_tools/src/base/process.dart'; | ||||||||||
import 'package:flutter_tools/src/cache.dart'; | ||||||||||
import 'package:flutter_tools/src/device.dart'; | ||||||||||
import 'package:flutter_tools/src/ios/ios_deploy.dart'; | ||||||||||
|
@@ -147,6 +148,28 @@ void main () { | |||||||||
expect(logger.traceText, contains('* thread #1')); | ||||||||||
}); | ||||||||||
|
||||||||||
testWithoutContext('debugger attached and stop failed', () async { | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this test verifying any of your changes here? It's passing for me if I revert your other changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, let me rework it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay updated it |
||||||||||
final StreamController<List<int>> stdin = StreamController<List<int>>(); | ||||||||||
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', | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity, would we actually expect to see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh good question, I'm not sure. I just grabbed that from the test above. I'll try and figure out There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not worth investigating, just wondering if there was a legit reason. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
|
||||||||||
stdin: IOSink(stdin.sink), | ||||||||||
), | ||||||||||
]); | ||||||||||
final IOSDeployDebuggerWaitForExit iosDeployDebugger = IOSDeployDebuggerWaitForExit.test( | ||||||||||
processManager: processManager, | ||||||||||
logger: logger, | ||||||||||
); | ||||||||||
|
||||||||||
expect(iosDeployDebugger.logLines, emitsInOrder(<String>[ | ||||||||||
'success', | ||||||||||
])); | ||||||||||
|
||||||||||
expect(await iosDeployDebugger.launchAndAttach(), isTrue); | ||||||||||
await iosDeployDebugger.exitCompleter.future; | ||||||||||
}); | ||||||||||
|
||||||||||
testWithoutContext('handle processing logging after process exit', () async { | ||||||||||
final StreamController<List<int>> stdin = StreamController<List<int>>(); | ||||||||||
// Make sure we don't hit a race where logging processed after the process exits | ||||||||||
|
@@ -591,3 +614,37 @@ IOSDeploy setUpIOSDeploy(ProcessManager processManager, { | |||||||||
cache: cache, | ||||||||||
); | ||||||||||
} | ||||||||||
|
||||||||||
class IOSDeployDebuggerWaitForExit extends IOSDeployDebugger { | ||||||||||
IOSDeployDebuggerWaitForExit({ | ||||||||||
required super.logger, | ||||||||||
required super.processUtils, | ||||||||||
required super.launchCommand, | ||||||||||
required super.iosDeployEnv | ||||||||||
}); | ||||||||||
|
||||||||||
/// Create a [IOSDeployDebugger] for testing. | ||||||||||
/// | ||||||||||
/// Sets the command to "ios-deploy" and environment to an empty map. | ||||||||||
factory IOSDeployDebuggerWaitForExit.test({ | ||||||||||
required ProcessManager processManager, | ||||||||||
Logger? logger, | ||||||||||
}) { | ||||||||||
final Logger debugLogger = logger ?? BufferLogger.test(); | ||||||||||
return IOSDeployDebuggerWaitForExit( | ||||||||||
logger: debugLogger, | ||||||||||
processUtils: ProcessUtils(logger: debugLogger, processManager: processManager), | ||||||||||
launchCommand: <String>['ios-deploy'], | ||||||||||
iosDeployEnv: <String, String>{}, | ||||||||||
); | ||||||||||
} | ||||||||||
|
||||||||||
final Completer<void> exitCompleter = Completer<void>(); | ||||||||||
|
||||||||||
@override | ||||||||||
bool exit() { | ||||||||||
final bool status = super.exit(); | ||||||||||
exitCompleter.complete(); | ||||||||||
return status; | ||||||||||
} | ||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason not to just call
dispose()
from thefinally
block?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want it to dispose when it launches successfully
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh yeah, good point :)