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.

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

[dwds] callServiceExtension should check all extensions and return kMethodNotFound when extension not found #2597

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

srujzs
Copy link
Contributor

@srujzs srujzs commented Mar 6, 2025

Currently, callServiceExtension checks the extensionRPCs field in the VM service. This list is populated when DWDS starts and checks the extensions currently registered in the runtime. This list is prone to becoming stale, however, as services may be registered or removed through the runtime (via dart:developer) later.

Instead, we should seek to always check the runtime for the current registered extensions and then update this list whenever we do. It’s still prone to becoming stale, but it is less stale. Ideally, we would override extensionRPCs and fetch the current registered extensions, but that isn’t possible since it requires awaiting a future. We also can’t ignore this field altogether because it looks like clients, like Dart DevTools, uses this field. Note that extensionRPCs should be a subset of getExtensionRpcs since registerService isn't supported in DWDS:

Future<Success> registerService(String service, String alias) {
.

callServiceExtension should also return kMethodNotFound instead of its current JS evaluation error when an extension is not present.

…ethodNotFound when extension not found and remove reassemble from reload

dart-lang#2584

Currently, callServiceExtension checks the extensionRpcs field in the VM
service. This list is populated when DWDS starts and checks the extensions
currently registered in the runtime. This list is prone to becoming stale,
however, as services may be registered or removed through the runtime
(via dart:developer) later. This is true for ext.flutter.reassemble, where DWDS
can not find the service because we computed the list too early.

Instead, we should seek to always check the runtime for the current registered
extensions and then update this list whenever we do. It’s still prone to becoming
stale, but it is less stale. Ideally, we would override extensionRpcs and fetch the
current registered extensions, but that isn’t possible since it requires awaiting a
future. We also can’t ignore this field altogether because it looks like clients, like
Dart DevTools, uses this field.

callServiceExtension should also return kMethodNotFound instead of its current
JS evaluation error when an extension is not present.

Lastly, the reassemble call is removed from reload as it can now be safely called in
Flutter tools instead.
@srujzs srujzs requested a review from bkonyi March 6, 2025 23:40
@srujzs srujzs changed the title [dwds] callServiceExtension should check all extensions and return kMethodNotFound when extension not found and remove reassemble from reload [dwds] callServiceExtension should check all extensions and return kMethodNotFound when extension not found Mar 7, 2025
@srujzs
Copy link
Contributor Author

srujzs commented Mar 7, 2025

I'm a little unsure what the potential ramifications of this are, but considering we're just keeping information more updated and returning errors that correspond to the spec, hopefully it should be safe.


This came up in #2585. The Flutter hot reload tests don't set up reassemble: https://github.com/flutter/flutter/blob/d14d2505f3aa748450ae11ca0996743c2eebf99a/packages/flutter_tools/test/integration.shard/test_data/hot_reload_test_common.dart#L1, so when using callServiceExtension, we return an unknown error due to a failed JS evaluation to call that service extension in the runtime. This then leads to test failures. Flutter tools ignores kMethodNotFound errors, but can and should not ignore unknown errors (like the failed evaluation).

So, the minimum needed to support these integration tests is for DWDS to return kMethodNotFound when the service extension is not found to align with the VM. Everything else in this PR is just to keep things less stale. I believe we can't really mock this service extension in the tests because we spin up a real Flutter process.

In general too, I've seen occasional logs pop up due to missing service extensions triggering errors when debugging random things for hot reload. It's hard to find a repro, but I suspect those should go away as well with this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant