Skip to content

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

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

Merged
merged 3 commits into from
Mar 11, 2025

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.

Copy link
Contributor

@jyameo jyameo left a comment

Choose a reason for hiding this comment

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

The changes in this PR look fairly straightforward to me. LGTM! (PS Ben is OOO this week)

@srujzs
Copy link
Contributor Author

srujzs commented Mar 11, 2025

Ah okay, thanks Jessy! I'll go ahead and land this.

@srujzs srujzs merged commit 3e17660 into dart-lang:main Mar 11, 2025
47 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Mar 13, 2025
Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/6c770bf..c41b86b):
  c41b86b9  2025-03-12  Sam Rawlins  Remove reference to AugmentableElement, soon to be deleted (dart-lang/dartdoc#4016)
  5dbd403c  2025-03-10  Parker Lougheed  Utilize scss features in sidebar styles (dart-lang/dartdoc#4010)

http (https://github.com/dart-lang/http/compare/900da9f..001665e):
  001665e  2025-03-11  dependabot[bot]  Bump the github-actions group across 1 directory with 5 updates (dart-lang/http#1723)

i18n (https://github.com/dart-lang/i18n/compare/bdeec25..b09c822):
  b09c822  2025-03-12  Moritz  Dont check hashes on tag
  f681aa0  2025-03-12  Moritz  Fix artifact building workflows (dart-lang/i18n#956)
  2644c17  2025-03-11  Moritz  Small fixes to intl4x (dart-lang/i18n#955)
  6d680ad  2025-03-11  Copybara-Service  Merge pull request `#948` from kszczek:ignore-script-subtags
  88ab7a8  2025-02-20  Kamil Szczęk  feat(intl): ignore script subtags when canonicalizing locale strings

tools (https://github.com/dart-lang/tools/compare/d67cd00..9c53358):
  9c533582  2025-03-11  Parker Lougheed  [package_config] Prepare for 2.2.0 release (dart-lang/tools#2031)

webdev (https://github.com/dart-lang/webdev/compare/f485686..302b6db):
  302b6db6  2025-03-11  Srujan Gaddam  Remove reassemble invocation and publish 24.3.7 (dart-lang/webdev#2598)
  3e17660f  2025-03-11  Srujan Gaddam  [dwds] callServiceExtension should check all extensions and return kMethodNotFound when extension not found (dart-lang/webdev#2597)
  8f146a15  2025-03-05  Srujan Gaddam  Roll dwds to 24.3.7-wip and webdev to 3.7.2-wip (dart-lang/webdev#2596)
  2c1d147d  2025-03-04  Srujan Gaddam  Add pull request and id-token write permissions to publish.yaml (dart-lang/webdev#2595)
  8a8eaf1b  2025-03-04  Srujan Gaddam  Add hotReloadSourcesUri parameter to FrontendServerDdcLibraryBundleProvider and publish DWDS 24.3.6 (dart-lang/webdev#2594)

Change-Id: I9b7b16a0c8f62cd70e1eb76dea485bbef7b8be15
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/415300
Commit-Queue: Devon Carew <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
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.

2 participants