Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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 dwds/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## 26.1.0-wip
## 26.1.0

- Added `NoClientsAvailableException` and structured error responses with `noClientsAvailable` field for graceful handling when no browser clients are connected during hot reload or hot restart operations.
- `pause` now does not send a `PauseInterrupted` event in
`WebSocketProxyService` as we didn't actually pause.

Expand Down
59 changes: 51 additions & 8 deletions dwds/lib/src/services/web_socket_proxy_service.dart
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,17 @@ class _ServiceExtensionTracker {
}
}

/// Exception thrown when no browser clients are connected to DWDS.
class NoClientsAvailableException implements Exception {
final String message;
final String operation;

NoClientsAvailableException(this.message, {required this.operation});

@override
String toString() => 'NoClientsAvailableException: $message';
}

/// WebSocket-based VM service proxy for web debugging.
class WebSocketProxyService extends ProxyService {
final _logger = Logger('WebSocketProxyService');
Expand Down Expand Up @@ -504,6 +515,14 @@ class WebSocketProxyService extends ProxyService {
await _performWebSocketHotReload();
_logger.info('Hot reload completed successfully');
return _ReloadReportWithMetadata(success: true);
} on NoClientsAvailableException catch (e) {
// Gracefully handle no clients scenario
_logger.info('No clients available for hot reload');
return _ReloadReportWithMetadata(
success: false,
notices: [e.message],
noClientsAvailable: true,
);
} catch (e) {
_logger.warning('Hot reload failed: $e');
return _ReloadReportWithMetadata(success: false, notices: [e.toString()]);
Expand All @@ -518,6 +537,16 @@ class WebSocketProxyService extends ProxyService {
await _performWebSocketHotRestart();
_logger.info('Hot restart completed successfully');
return {'result': vm_service.Success().toJson()};
} on NoClientsAvailableException catch (e) {
// Return structured response indicating no clients available
_logger.info('No clients available for hot restart');
return {
'result': {
'type': 'Success',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't feel right as this isn't a Success.

Realistically, we should be doing something like throwing an RPCError with an error code that package:vm_service forwards without modification. Right now, package:vm_service catches all exceptions and wraps them in RPCErrors with a kServerError code, but maybe we should update it to rethrow RPCError instances.

Another option would be to update the hotRestart extension to return ReloadReport as well so we can be consistent between hotReload and hotRestart.

Copy link
Contributor Author

@jyameo jyameo Oct 15, 2025

Choose a reason for hiding this comment

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

Good catch! This shouldn’t be treated as a success. I think returning a ReloadReport would be the better approach to keep things consistent and also add the field noClientsAvailable. For now, I’ve removed the type field since it’s not being used for this operation.

In another PR, we can refactor this to return a ReloadReport instead, that would also involve updating the Chrome path and Flutter tools as well, as they currently expect the existing format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does that sound? @bkonyi

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually don't think we can remove the type property in the result without consequence. package:vm_service checks for this and, if it can't find the type, returns a Response by default. While I don't think anyone is checking for the Success object explicitly since there's no useful information attached to it, this could break things.

I think it's just worth updating the return type to be ReloadReport now and then updating Flutter to return the same when we roll this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following our discussion I have made the changes to throw an RPC Error for this scenario which we catch in Flutter tools. PTAL

@bkonyi @srujzs

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I feel like this resurrects the original issue we had where we can't distinguish what the kServerError corresponds to, whereas if we had hot restart return a ReloadReport, we have that metadata in the report.

(also I'll be OOO tomorrow, so if you decide this is the right approach anyways, feel free to land it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Srujan, Ben sand I discussed this and decided that this approach to throw an RPCError would work better since changing hot restart to return a ReloadReport would also require modifying the Chrome path, which could introduce some breakage.

In Flutter tools, I check for the calling method and kServerError to distinguish that this is the specific scenario. We could technically add a new RPCErrorKind for this, but I’m not sure it’s really necessary.

Thoughts? @bkonyi

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we'd like to avoid using the generic kServerError, I think we can go ahead and use kIsolateCannotReload to indicate that the reload has failed.

'noClientsAvailable': true,
'message': e.message,
},
};
} catch (e) {
_logger.warning('Hot restart failed: $e');
return {
Expand Down Expand Up @@ -611,7 +640,11 @@ class WebSocketProxyService extends ProxyService {
});

if (clientCount == 0) {
throw StateError('No clients available for hot reload');
_logger.warning('No clients available for hot reload');
throw NoClientsAvailableException(
'No clients available for hot reload',
operation: 'hot reload',
);
}

// Create tracker for this hot reload request
Expand Down Expand Up @@ -671,7 +704,11 @@ class WebSocketProxyService extends ProxyService {
});

if (clientCount == 0) {
throw StateError('No clients available for hot restart');
_logger.warning('No clients available for hot restart');
throw NoClientsAvailableException(
'No clients available for hot restart',
operation: 'hot restart',
);
}

// Create tracker for this hot restart request
Expand Down Expand Up @@ -737,9 +774,8 @@ class WebSocketProxyService extends ProxyService {
final request = ServiceExtensionRequest.fromArgs(
id: requestId,
method: method,
args: args != null
? Map<String, dynamic>.from(args)
: <String, dynamic>{},
args:
args != null ? Map<String, dynamic>.from(args) : <String, dynamic>{},
);

// Send the request and get the number of connected clients
Expand Down Expand Up @@ -940,8 +976,8 @@ class WebSocketProxyService extends ProxyService {
/// Pauses execution of the isolate.
@override
Future<Success> pause(String isolateId) =>
// Can't pause with the web socket implementation, so do nothing.
Future.value(Success());
// Can't pause with the web socket implementation, so do nothing.
Future.value(Success());

/// Resumes execution of the isolate.
@override
Expand Down Expand Up @@ -1049,13 +1085,20 @@ class WebSocketProxyService extends ProxyService {
/// Extended ReloadReport that includes additional metadata in JSON output.
class _ReloadReportWithMetadata extends vm_service.ReloadReport {
final List<String>? notices;
_ReloadReportWithMetadata({super.success, this.notices});
final bool noClientsAvailable;

_ReloadReportWithMetadata({
super.success,
this.notices,
this.noClientsAvailable = false,
});

@override
Map<String, dynamic> toJson() {
final jsonified = <String, Object?>{
'type': 'ReloadReport',
'success': success ?? false,
'noClientsAvailable': noClientsAvailable,
};
if (notices != null) {
jsonified['notices'] = notices!.map((e) => {'message': e}).toList();
Expand Down
2 changes: 1 addition & 1 deletion dwds/lib/src/version.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dwds/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: dwds
# Every time this changes you need to run `dart run build_runner build`.
version: 26.1.0-wip
version: 26.1.0

description: >-
A service that proxies between the Chrome debug protocol and the Dart VM
Expand Down
Loading