Skip to content
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.

Sign up for GitHub

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

Jump to bottom

allow DevTools to re-connect to a new app after closing a previous app #1898

Merged
merged 4 commits into from
May 2, 2020
Merged

allow DevTools to re-connect to a new app after closing a previous app #1898

merged 4 commits into from
May 2, 2020

Conversation

devoncarew
Copy link
Member

@devoncarew devoncarew commented May 1, 2020 •
edited
Loading

This does some surgery on the ServiceConnectionManager class to make one Completer private, and ensure that we reset some minimal state between connections.

jacob314
jacob314 reviewed May 1, 2020
View reviewed changes
packages/devtools_app/lib/src/service_manager.dart Outdated
final serviceAvailable = Completer<void>();
Completer<VmService> _serviceAvailable = Completer();

Future get onServiceAvailable => _serviceAvailable.future;
Copy link
Contributor

Choose a reason for hiding this comment

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

missing type.
Future --> Future<VmService>

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

jacob314
jacob314 reviewed May 1, 2020
View reviewed changes
packages/devtools_app/lib/src/service_manager.dart
@@ -138,7 +142,7 @@ class ServiceConnectionManager {
serviceTrafficLogger = VmServiceTrafficLogger(service);
}

serviceAvailable.complete();
_serviceAvailable.complete(service);
Copy link
Contributor

Choose a reason for hiding this comment

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

we might want to also add

if (_serviceAvaillabe.isCompleted) {_serviceAvailable = Completer();}

at the start of this method to further support opening a new service when there was an existing service. Maybe just add a TODO because we would really want to further guard things so only one request to switch to a new vm service could be triggered at once.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we're actively connected to a we likely want to guard against opening a new connection - I expect we'll first want to disconnect from the existing connection?

I'll add a TODO:, and open an issue about having a way to disconnect from the current connection in the UI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the multiple connects issue to #1897, and filed #1900 about disconnecting from the current connection.

jacob314
jacob314 approved these changes May 1, 2020
View reviewed changes
Copy link
Contributor

@jacob314 jacob314 left a comment

Choose a reason for hiding this comment

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

lgtm

devoncarew
devoncarew commented May 1, 2020
View reviewed changes
Copy link
Member Author

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

Comments addressed, though I need to fix some tests before landing this.

packages/devtools_app/lib/src/service_manager.dart Outdated
final serviceAvailable = Completer<void>();
Completer<VmService> _serviceAvailable = Completer();

Future get onServiceAvailable => _serviceAvailable.future;
Copy link
Member Author

Choose a reason for hiding this comment

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

updated

packages/devtools_app/lib/src/service_manager.dart
@@ -138,7 +142,7 @@ class ServiceConnectionManager {
serviceTrafficLogger = VmServiceTrafficLogger(service);
}

serviceAvailable.complete();
_serviceAvailable.complete(service);
Copy link
Member Author

Choose a reason for hiding this comment

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

If we're actively connected to a we likely want to guard against opening a new connection - I expect we'll first want to disconnect from the existing connection?

I'll add a TODO:, and open an issue about having a way to disconnect from the current connection in the UI.

packages/devtools_app/lib/src/service_manager.dart
@@ -138,7 +142,7 @@ class ServiceConnectionManager {
serviceTrafficLogger = VmServiceTrafficLogger(service);
}

serviceAvailable.complete();
_serviceAvailable.complete(service);
Copy link
Member Author

Choose a reason for hiding this comment

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

Added the multiple connects issue to #1897, and filed #1900 about disconnecting from the current connection.

@devoncarew devoncarew merged commit 7d53611 into flutter:master May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacob314 jacob314 jacob314 approved these changes

@kenzieschmoll kenzieschmoll kenzieschmoll approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to connect to a second VM service after the first one disconnects
3 participants
@devoncarew @jacob314 @kenzieschmoll