Skip to content

Do not mark service extensions as 'added' so eagerly #9271

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

Merged
srawlins merged 9 commits into from
Jul 1, 2025
Merged

Do not mark service extensions as 'added' so eagerly #9271

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
28368b5
Do not mark service extensions as 'added' so eagerly
srawlins Jun 24, 2025
a755398
release-notes
srawlins Jun 24, 2025
55e3de0
feedback
srawlins Jun 27, 2025
c5a6612
feedback
srawlins Jun 30, 2025
d037261
Avoid bug when name is not in serviceExtensionsAllowlist
srawlins Jul 1, 2025
5c5e0aa
feedback
srawlins Jul 1, 2025
c720b1a
Update packages/devtools_app_shared/lib/src/service/service_extension…
srawlins Jul 1, 2025
ea40730
feedback
srawlins Jul 1, 2025
6cc4976
feedback
srawlins Jul 1, 2025
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
3 changes: 2 additions & 1 deletion packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ TODO: Remove this section if there are not any general updates.

## Network profiler updates

TODO: Remove this section if there are not any general updates.
* Fixed network logging after a hot restart. -
[#9271](https://github.com/flutter/devtools/pull/9271).

## Logging updates

Expand Down
79 changes: 50 additions & 29 deletions packages/devtools_app_shared/lib/src/service/service_extension_manager.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ final class ServiceExtensionManager with DisposerMixin {
}

Future<void> _addServiceExtension(String name) async {
if (!_serviceExtensions.add(name)) {
if (_serviceExtensions.contains(name)) {
// If the service extension was already added we do not need to add it
// again. This can happen depending on the timing between when extension
// added events were received and when we requested the list of all
Expand All @@ -254,68 +254,79 @@ final class ServiceExtensionManager with DisposerMixin {
}
_hasServiceExtension(name).value = true;

if (_enabledServiceExtensions.containsKey(name)) {
final enabledServiceExtension = _enabledServiceExtensions[name];
if (enabledServiceExtension != null) {
// Restore any previously enabled states by calling their service
// extension. This will restore extension states on the device after a hot
// restart. [_enabledServiceExtensions] will be empty on page refresh or
// initial start.
try {
return await _callServiceExtension(
final called = await _callServiceExtension(
name,
_enabledServiceExtensions[name]!.value,
enabledServiceExtension.value,
);
if (called) {
Comment thread
srawlins marked this conversation as resolved.
_serviceExtensions.add(name);
}
return;
} on SentinelException catch (_) {
// Service extension stopped existing while calling, so do nothing.
// This typically happens during hot restarts.
}
} else {
// Set any extensions that are already enabled on the device. This will
// enable extension states in DevTools on page refresh or initial start.
return await _restoreExtensionFromDevice(name);
final restored = await _restoreExtensionFromDevice(name);
if (restored) {
_serviceExtensions.add(name);
Comment thread
srawlins marked this conversation as resolved.
}
}
}

IsolateRef? get _mainIsolate => _isolateManager.mainIsolate.value;

Future<void> _restoreExtensionFromDevice(String name) async {
/// Restores the service extension named [name] from the device.
///
/// Returns whether the service extension was actually restored.
Future<bool> _restoreExtensionFromDevice(String name) async {
Comment thread
srawlins marked this conversation as resolved.
Outdated
final isolateRef = _isolateManager.mainIsolate.value;
if (isolateRef == null) return;
if (isolateRef == null) return false;

if (!extensions.serviceExtensionsAllowlist.containsKey(name)) {
return;
return false;
}
final expectedValueType =
extensions.serviceExtensionsAllowlist[name]!.values.first.runtimeType;

Future<void> restore() async {
Future<bool> restore() async {
// The restore request is obsolete if the isolate has changed.
if (isolateRef != _mainIsolate) return;
if (isolateRef != _mainIsolate) return false;
try {
final response = await _service!.callServiceExtension(
name,
isolateId: isolateRef.id,
);

if (isolateRef != _mainIsolate) return;
if (isolateRef != _mainIsolate) return false;

switch (expectedValueType) {
case const (bool):
final enabled = response.json!['enabled'] == 'true' ? true : false;
await _maybeRestoreExtension(name, enabled);
Copy link
Copy Markdown
Member

@kenzieschmoll kenzieschmoll Jun 30, 2025

Choose a reason for hiding this comment

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

should _maybeRestoreExtension return a bool, and then we can return await _maybeRestoreExtension? Not sure if if matters that we don't always call the extension from _maybeRestoreExtension

Copy link
Copy Markdown
Contributor Author

@srawlins srawlins Jun 30, 2025

Choose a reason for hiding this comment

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

If we have _maybeRestoreExtension return a bool, then it will always return true, which I think would be silly, or it could delegate the return value to the two setServiceExtensionState calls it makes, but that function would also just always return true.

Copy link
Copy Markdown
Member

@kenzieschmoll kenzieschmoll Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think _maybeRestoreExtension would always return true. There is a logic path in that method that is a no-op (when the nested if condition is not met).

Copy link
Copy Markdown
Contributor Author

@srawlins srawlins Jul 1, 2025

Choose a reason for hiding this comment

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

Ah you're right. OK I've updated the code to use the return value from here.

return;
return true;
case const (String):
final String? value = response.json!['value'];
await _maybeRestoreExtension(name, value);
return;
return true;
case const (int):
case const (double):
final value = num.parse(
response.json![name.substring(name.lastIndexOf('.') + 1)],
);
await _maybeRestoreExtension(name, value);
return;
return true;
default:
return;
return false;
Comment thread
srawlins marked this conversation as resolved.
Outdated
}
} catch (e) {
// Do not report an error if the VMService has gone away or the
Copy link
Copy Markdown
Member

@kenzieschmoll kenzieschmoll Jul 1, 2025

Choose a reason for hiding this comment

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

should we still return false on catch? or at least when the error indicates the service is disposed? See isServiceDisposedError helper

Copy link
Copy Markdown
Contributor Author

@srawlins srawlins Jul 1, 2025

Choose a reason for hiding this comment

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

I really don't understand the lifecycle of when service extensions are added, or restored. For example I don't know why the previous changes caused test timeouts. The comment for the catch has an incomplete thought:

// Do not report an error if the VMService has gone away or the
// selectedIsolate has been closed probably due to a hot restart.
// There is no need

If the bool return value essentially means "don't retry this operation later," like true = "service extension was restored, or some other state that means you shouldn't retry," and false = "service extension was not restored, and it's maybe safe to try again," then it sounds like your idea is sound. We could do something like if (e is RPCError && e.isServiceDisposedError) return false;, or something similar?

Copy link
Copy Markdown
Member

@kenzieschmoll kenzieschmoll Jul 1, 2025

Choose a reason for hiding this comment

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

Yeah we can do:

on RPCError catch (e) {
  if (e.isServiceDisposedError) {
    return false;
  }
} catch (e) {
  // swallow
}

Copy link
Copy Markdown
Contributor Author

@srawlins srawlins Jul 1, 2025

Choose a reason for hiding this comment

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

Ok, I've moved that extension getter into package:devtools_app_shared, into a new library, exported publicly. Let me know if any of my choices there don't fit the style.

Expand All @@ -325,12 +336,13 @@ final class ServiceExtensionManager with DisposerMixin {
// of allowed network related exceptions rather than ignoring all
// exceptions.
}
return false;
}

if (isolateRef != _mainIsolate) return;
if (isolateRef != _mainIsolate) return false;

final isolate = await _isolateManager.isolateState(isolateRef).isolate;
if (isolateRef != _mainIsolate) return;
if (isolateRef != _mainIsolate) return false;

// Do not try to restore Dart IO extensions for a paused isolate.
if (extensions.isDartIoExtension(name) &&
Expand All @@ -339,6 +351,8 @@ final class ServiceExtensionManager with DisposerMixin {
} else {
await restore();
Comment thread
srawlins marked this conversation as resolved.
Outdated
Comment thread
srawlins marked this conversation as resolved.
Outdated
}

return true;
}

Future<void> _maybeRestoreExtension(String name, Object? value) async {
Expand All @@ -362,14 +376,15 @@ final class ServiceExtensionManager with DisposerMixin {
}
}

Future<void> _callServiceExtension(String name, Object? value) async {
if (_service == null) {
return;
}
/// Calls the service extension named [name] with [value].
///
/// Returns whether the service extension was successfully called.
Future<bool> _callServiceExtension(String name, Object? value) async {
if (_service == null) return false;

final mainIsolate = _isolateManager.mainIsolate.value;
Future<void> callExtension() async {
if (_isolateManager.mainIsolate.value != mainIsolate) return;
final mainIsolate = _mainIsolate;
Future<bool> callExtension() async {
if (_mainIsolate != mainIsolate) return false;

assert(value != null);
try {
Expand Down Expand Up @@ -408,28 +423,34 @@ final class ServiceExtensionManager with DisposerMixin {
// of the extension name (ext.flutter.extensionName => extensionName).
args: {name.substring(name.lastIndexOf('.') + 1): value},
);
} else {
return false;
Comment thread
srawlins marked this conversation as resolved.
Outdated
}
Comment thread
srawlins marked this conversation as resolved.
} on RPCError catch (e) {
if (e.code == RPCErrorKind.kServerError.code) {
// Connection disappeared
return;
// The connection disappeared.
return false;
}
rethrow;
}

return true;
}

if (mainIsolate == null) return;
if (mainIsolate == null) return false;

final isolate = await _isolateManager.isolateState(mainIsolate).isolate;
if (_isolateManager.mainIsolate.value != mainIsolate) return;
if (_mainIsolate != mainIsolate) return false;

// Do not try to call Dart IO extensions for a paused isolate.
if (extensions.isDartIoExtension(name) &&
isolate?.pauseEvent?.kind?.contains('Pause') == true) {
_callbacksOnIsolateResume
.putIfAbsent(mainIsolate, () => [])
.add(callExtension);
return true;
} else {
await callExtension();
return await callExtension();
}
}

Expand Down
Loading