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 6 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
107 changes: 72 additions & 35 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,90 @@ 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 _callServiceExtensionIfReady(
name,
_enabledServiceExtensions[name]!.value,
enabledServiceExtension.value,
);
if (called) {
Comment thread
srawlins marked this conversation as resolved.
// Only mark `name` as an "added service extension" if it was truly
// added. If it was added, then subsequent calls to
// `_addServiceExtension` with `name` will return early. If it was not
// really added, then subsequent calls to `_addServiceExtension` with
// `name` will proceed as usual.
_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 _restoreExtensionFromDeviceIfReady(name);
if (restored) {
// Only mark `name` as an "added service extension" if it was truly
// restored. If it was restored, then subsequent calls to
// `_addServiceExtension` with `name` will return early. If it was not
// really restored, then subsequent calls to `_addServiceExtension`
// with `name` will proceed as usual.
_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 isolates in the test app are prepared for the restore.
Comment thread
srawlins marked this conversation as resolved.
Outdated
Comment thread
srawlins marked this conversation as resolved.
Outdated
Future<bool> _restoreExtensionFromDeviceIfReady(String name) async {
final isolateRef = _isolateManager.mainIsolate.value;
if (isolateRef == null) return;
if (isolateRef == null) return false;

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

Future<void> restore() async {
/// Restores the service extension named [name].
///
/// Returns whether isolates in the connected app are prepared for the
/// restore.
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);
return;
return await _maybeRestoreExtension(name, enabled);
case const (String):
final String? value = response.json!['value'];
await _maybeRestoreExtension(name, value);
return;
return await _maybeRestoreExtension(name, value);
case const (int):
case const (double):
final value = num.parse(
response.json![name.substring(name.lastIndexOf('.') + 1)],
);
await _maybeRestoreExtension(name, value);
return;
return await _maybeRestoreExtension(name, value);
default:
return;
return true;
}
} 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,23 +347,28 @@ final class ServiceExtensionManager with DisposerMixin {
// of allowed network related exceptions rather than ignoring all
// exceptions.
}
return true;
}

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) &&
isolate?.pauseEvent?.kind?.contains('Pause') == true) {
_callbacksOnIsolateResume.putIfAbsent(isolateRef, () => []).add(restore);
return true;
} else {
await restore();
return await restore();
}
}

Future<void> _maybeRestoreExtension(String name, Object? value) async {
/// Maybe restores the service extension named [name] with [value].
///
/// Returns whether the service extension's state is set.
Comment thread
srawlins marked this conversation as resolved.
Outdated
Future<bool> _maybeRestoreExtension(String name, Object? value) async {
final extensionDescription = extensions.serviceExtensionsAllowlist[name];
if (extensionDescription is extensions.ToggleableServiceExtension) {
if (value == extensionDescription.enabledValue) {
Expand All @@ -351,25 +378,29 @@ final class ServiceExtensionManager with DisposerMixin {
value: value,
callExtension: false,
);
return true;
}
return false;
} else {
await setServiceExtensionState(
name,
enabled: true,
value: value,
callExtension: false,
);
return true;
}
}

Future<void> _callServiceExtension(String name, Object? value) async {
if (_service == null) {
return;
}
/// Calls the service extension named [name] with [value].
///
/// Returns whether isolates in the connected app are prepared for the call.
Future<bool> _callServiceExtensionIfReady(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 +439,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 Expand Up @@ -488,7 +525,7 @@ final class ServiceExtensionManager with DisposerMixin {
bool callExtension = true,
}) async {
if (callExtension && _serviceExtensions.contains(name)) {
await _callServiceExtension(name, value);
await _callServiceExtensionIfReady(name, value);
} else if (callExtension) {
_log.info(
'Attempted to call extension \'$name\', but no service with that name exists',
Expand Down