Skip to content

Ensure attaching to an application with an existing DDS instance is not treated as a fatal error - #70847

Merged
bkonyi merged 7 commits into
masterfrom
fix_existing_dds_attach_retry
Dec 5, 2020
Merged

Ensure attaching to an application with an existing DDS instance is not treated as a fatal error#70847
bkonyi merged 7 commits into
masterfrom
fix_existing_dds_attach_retry

Conversation

@bkonyi

@bkonyi bkonyi commented Nov 19, 2020

Copy link
Copy Markdown
Contributor

@DanTup, this should fix Dart-Code/Dart-Code#2936.

Re-upload of #70792 since I apparently don't know how to Git.

@flutter-dashboard flutter-dashboard Bot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Nov 19, 2020
@bkonyi
bkonyi requested a review from jonahwilliams November 19, 2020 17:33
@google-cla google-cla Bot added the cla: yes label Nov 19, 2020
@bkonyi bkonyi mentioned this pull request Nov 19, 2020
Closed
jonahwilliams
jonahwilliams approved these changes Nov 19, 2020
View reviewed changes

@jonahwilliams jonahwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jonahwilliams jonahwilliams mentioned this pull request Nov 21, 2020
Closed
@DanTup

DanTup commented Nov 30, 2020

Copy link
Copy Markdown
Contributor

I tried merging these changes locally for testing, though I still see this issue (it prints it as a warning, but then also as an error and quits):

Screenshot 2020-11-30 at 11 59 35

If I reproduce using just the terminal, the process doesn't quit (it's probably VS Code doing that when it gets the app.stop event) but you still see the error and the stop message:

danny@Dannys-MBP asdassdadsasda % flutter attach --machine -d macos --debug-uri http://127.0.0.1:51289/OVnnaegeXoQ=/   
[{"event":"daemon.connected","params":{"version":"0.6.0","pid":4710}}]
[{"event":"app.start","params":{"appId":"a35caf97-ca20-42a7-8fa3-c7f7fa8f62b0","deviceId":"macos","directory":null,"supportsRestart":true,"launchMode":"attach"}}]
Error connecting to the service protocol: failed to connect to http://127.0.0.1:51289/OVnnaegeXoQ=/
[{"event":"app.stop","params":{"appId":"a35caf97-ca20-42a7-8fa3-c7f7fa8f62b0"}}]

@jonahwilliams

jonahwilliams commented Dec 3, 2020

Copy link
Copy Markdown
Contributor

FYI @bkonyi I'll be OOO if you get time to come back to this.

@bkonyi
bkonyi force-pushed the fix_existing_dds_attach_retry branch from 51381ee to d5c30be Compare December 3, 2020 21:43
@bkonyi

bkonyi commented Dec 3, 2020

Copy link
Copy Markdown
Contributor Author

Confirmed the behavior you're seeing @DanTup. I'll take a look today.

jonahwilliams
jonahwilliams reviewed Dec 4, 2020
View reviewed changes
Comment thread packages/flutter_tools/lib/src/base/dds.dart
} on dds.DartDevelopmentServiceException catch (e) {
logger.printTrace('Warning: Failed to start DDS: ${e.message}');
if (e.errorCode == dds.DartDevelopmentServiceException.existingDdsInstanceError) {
_existingDdsUri = Uri.parse(

@jonahwilliams jonahwilliams Dec 4, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a cleanup it would be nice to provide this as an optional field on the DartDevelopmentServiceException

jonahwilliams
jonahwilliams approved these changes Dec 4, 2020
View reviewed changes

@jonahwilliams jonahwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM

@bkonyi
bkonyi force-pushed the fix_existing_dds_attach_retry branch from b1712e1 to 165413f Compare December 5, 2020 00:13
@bkonyi
bkonyi merged commit 89ef88c into master Dec 5, 2020
@bkonyi
bkonyi deleted the fix_existing_dds_attach_retry branch December 5, 2020 01:16
@DanTup

DanTup commented Dec 7, 2020

Copy link
Copy Markdown
Contributor

Works now, thanks!

@DanTup DanTup mentioned this pull request Dec 7, 2020
Closed
@zanderso zanderso mentioned this pull request Dec 15, 2020
Closed
@DanTup DanTup mentioned this pull request Aug 25, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@jonahwilliams jonahwilliams jonahwilliams approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Attaching to flutter tests no longer works

3 participants

@bkonyi @DanTup @jonahwilliams