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

Fix a bug where we were unnecessarily calling manualDisconnect #6700

Closed
wants to merge 1 commit into from
Closed

Fix a bug where we were unnecessarily calling manualDisconnect #6700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/devtools_app/lib/src/app.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
Map<String, String?> args,
DevToolsNavigationState? state,
) {
if (FrameworkCore.initializationInProgress) {
if (FrameworkCore.vmServiceConnectionInProgress) {
return const MaterialPage(child: CenteredCircularProgressIndicator());
}

Expand Down
6 changes: 3 additions & 3 deletions packages/devtools_app/lib/src/framework/framework_core.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class FrameworkCore {
_log.info('DevTools version ${devtools.version}.');
}

static bool initializationInProgress = false;
static bool vmServiceConnectionInProgress = false;

/// Returns true if we're able to connect to a device and false otherwise.
static Future<bool> initVmService(
Expand All @@ -70,7 +70,7 @@ class FrameworkCore {
final normalizedUri = normalizeVmServiceUri(serviceUriAsString);
final Uri? uri = normalizedUri ?? getServiceUriFromQueryString(url);
if (uri != null) {
initializationInProgress = true;
vmServiceConnectionInProgress = true;
final finishedCompleter = Completer<void>();

try {
Expand Down Expand Up @@ -111,7 +111,7 @@ class FrameworkCore {
errorReporter!('Unable to connect to VM service at $uri: $e', e);
return false;
} finally {
initializationInProgress = false;
vmServiceConnectionInProgress = false;
}
} else {
// Don't report an error here because we do not have a URI to connect to.
Expand Down
4 changes: 3 additions & 1 deletion packages/devtools_app/lib/src/shared/routing.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ class DevToolsRouteInformationParser
// If the uri has been modified and we do not have a vm service uri as a
// query parameter, ensure we manually disconnect from any previously
// connected applications.
await serviceConnection.serviceManager.manuallyDisconnect();
if (serviceConnection.serviceManager.hasConnection) {
await serviceConnection.serviceManager.manuallyDisconnect();
}
Comment on lines +63 to +65
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too familiar with this, but is it possible we'd have a race here if there is a connection in progress (hasConnection being false so we skip this, but a connection ends up being formed shortly after?)

(if it is possible, I don't know if calling manuallyDisconnect() is the right thing to do to cancel that anyway, so it might be an unrelated issue if there is such an issue)

} else if (_forceVmServiceUri == null) {
// Otherwise, connect to the vm service from the query parameter before
// loading the route (but do not do this in a testing environment).
Expand Down
Loading