Skip to content

Support running DWDS without a Chrome Debug Port (web-socket-based) #2639

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

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

Conversation

jyameo
Copy link
Contributor

@jyameo jyameo commented Jun 26, 2025

  • Implemented a WebSocket-based communication protocol that provides essential developer tooling (hot reload, service extensions) when Chrome debugger access is unavailable. - #2605
  • Added WebSocket-based hot reload and service extension support via new WebSocketProxyService class that implements VM service protocol over WebSockets.
  • Created new files: WebSocketProxyService, WebSocketDebugService, WebSocketAppDebugServices, and WebSocketDwdsVmClient to support socket-based DWDS functionality.
  • Enhanced DevHandler with useWebSocketConnection flag to toggle between Chrome-based and WebSocket-based communication protocols.

Required Flutter Tools changes: flutter/flutter#171648

@jyameo jyameo requested review from srujzs and nshahan July 18, 2025 19:12
Copy link
Contributor

Choose a reason for hiding this comment

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

As a follow-up (in a different change) should we update the ChromeProxyService to also use this Built object for service extensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ChromeProxyService leverages the inspector to evaluate JavaScript expressions via invokeExtensionJsExpression (as seen in the _callServiceExtension method). This pattern is consistently used throughout ChromeProxyService for all interactions with the dartDevEmbedder. It evaluates JavaScript expressions directly in the browser context rather than sending structured messages. Since ChromeProxyService operates by executing JavaScript in the browser, using a Built object here wouldn't align with ChromeProxyService's JavaScript evaluation approach. The current pattern maintains consistency with how ChromeProxyService handles all other browser interactions.

remoteDebugger.onClose.first.then((_) => close());
}
} catch (_) {
// Chrome proxy service not available in WebSocket-only mode - ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

What exceptions are we now catching and ignoring here?

If we're in "websocket-only mode" (does this mean -d web-server?) then I would expect the debugger to never even be connected. But if we're not in websocket-only mode (-d chrome?) then it seems like it should at least be a warning if we get an exception here.

Can you add more context on what this is doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was because we were throwing an UnsupportedError in the web_socket_app_debug_services when accessing chromeProxyService. I have changed the logic to return null instead, so we can now simply do: _appDebugServices.chromeProxyService?.remoteDebugger.onClose.first.then((_) { close(); });

Copy link
Contributor

@nshahan nshahan Jul 21, 2025

Choose a reason for hiding this comment

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

I believe there could be a debugger connected when in "web-server" mode after the application starts if the user manually connects it using the Dart DevTools chrome extension.

Actually now I think I agree, what errors are we swallowing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, the UnsupportedError was being thrown when accessing chromeProxyService from web_socket_app_debug_services, which isn't supposed to happen since that service doesn’t support Chrome debugging. I’ve since refactored the code so both app_debug_services and web_socket_app_debug_services implement a common interface. As part of that, chromeProxyService has been removed in favour of a unified proxyService interface, which can be type-checked as needed.

@@ -22,9 +22,20 @@ class DebugConnection {
Future<void>? _closed;

DebugConnection(this._appDebugServices) {
_appDebugServices.chromeProxyService.remoteDebugger.onClose.first.then((_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this all just be:

_appDebugServices.chromeProxyService?.remoteDebugger.onClose.first.then((_) { close(); });

All the new code here looks to be doing all the same checks as close below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! I've simplified the constructor now

} on StateError catch (e) {
// The sink has already closed (app is disconnected), or another StateError occurred.
_logger.warning(
'Failed to send request to client, connection likely closed. Error: $e',
'Failed to send request to client ${injectedConnection.hashCode}, '
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the hashCode actually add any information to these messages? I'm assuming the injected clients don't override hashCode so it'll just be a random number every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, you are right! I was using the hashCode as a simple way to differentiate between multiple connections in log messages, but it's not very useful since there is no persistence and it's hard to track.

Future<Map<String, dynamic>?> handleServiceExtension(
String method,
Map<String, dynamic> args,
) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a missing await somewhere in this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! thanks

await webSocketProxyService.isInitialized;
_logger.fine('WebSocket proxy service initialized successfully');
} else {
_logger.warning('WebSocket proxy service is null');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there ever an instance where this is okay? Should we throw/assert instead? Similar question below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! I have updated the code to use try/catch/throw instead!

appConnection.runMain();
_mainHasStarted = true;
} catch (e) {
if (e.toString().contains('Main has already started')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to catch this? I would assume the check on line 900 prevents this from occurring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the try/catch block is needed here to handle race conditions where multiple concurrent resume() calls can pass the if (!_mainHasStarted) check before any of them complete and set _mainHasStarted = true

@jyameo jyameo requested review from biggs0125, srujzs and nshahan July 23, 2025 15:35
///
/// All subsequent calls to [close] will return this future.
/// Chrome-based debug services container.
class AppDebugServices implements IAppDebugServices {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to name this ChromeAppDebugServices and the interface AppDebugServices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. I have updated the name for the classes and interface to be more clear.

port: port,
path: authToken,
),
serviceUri: Uri(scheme: 'http', host: hostname, port: ddsPort ?? 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is 0 a valid default port realistically? Does it make sense to require a ddsPort so we don't need to try and pick a default here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is what we have been doing for chrome previous to my changes so I kept the existing behavior

webSocketProxyService,
);

final server = await startHttpServer(hostname, port: 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar question here. Why are we using port: 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Good catch here. I updated this to be port: 44456 which is what we currently have for chrome

@jyameo jyameo requested a review from nshahan July 23, 2025 19:03
Copy link
Contributor

@nshahan nshahan left a comment

Choose a reason for hiding this comment

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

Thanks @jyameo! Pending final round of reviews but LGTM.

/// A Dart Web Debug Service.
///
/// Creates a [ChromeProxyService] from an existing Chrome instance.
class DebugService {
class ChromeDebugService implements DebugService {
static String? _ddsUri;

final VmServiceInterface chromeProxyService;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this should be typed as ChromeProxyService chromeProxyService


@override
ProxyService get proxyService =>
debugService.chromeProxyService as ChromeProxyService;
Copy link
Contributor

Choose a reason for hiding this comment

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

See below, think we can get rid of this cast.

final WebSocketDebugService _debugService;
final WebSocketDwdsVmClient _dwdsVmClient;
Future<void>? _closed;
String? _connectedInstanceId;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should just be able to change this to:

@override
String? connectedInstanceId;

and get rid of the getter and setter below.

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.

4 participants