[vm_service] VmServerConnection does not handle SentinelExceptions as prescribed by the vm service interface

[annagrin]

VmServerConnection seems to convert SentinelException to an RPC error. This has caused flakes in flutter web tests:

Parent issue: flutter/flutter#121708

The issue above currently has a PR that resolves it for one case, but we should have a reliable solution that works for all vm service API and their clients, to prevent more hard-to-detect failures due to inconsistencies in checks.

Details

dwds' chrome proxy service throws a sentinel exception on getIsolate for non-existing isolate id, as required by the VmServiceInterface:

sdk/pkg/vm_service/lib/src/vm_service.dart

Line 690 in 67f3e1f

Future<Isolate> getIsolate(String isolateId);

but the VmServerConnection translates it into an RPC error with RPCError.kInternalError as code, and the sentinel is added to the message as a string:

sdk/pkg/vm_service/lib/src/vm_service.dart

Line 1795 in 67f3e1f

final error = e is RPCError

The clients then need to check for the string contents to contain a sentinel kind. See flutter/flutter#122776 (comment) for a client check example.

Should we change the VmServerConnection to throw something different, or have a set of error codes in package:vm_service we all agree on in servers and clients can check for in their code?

[annagrin]

/cc @bkonyi @derekxu16

[derekxu16]

Here's my analysis of the problem:

flutter_tools passes the VmServerConnection URI that DWDS creates to createVmServiceDelegate here
https://github.com/flutter/flutter/blob/23f5582ea654d01f9462f04158cf2c2a188708ec/packages/flutter_tools/lib/src/isolated/devfs_web.dart#L697

createVmServiceDelegate uses the URI to create a VmService object here
https://github.com/flutter/flutter/blob/23f5582ea654d01f9462f04158cf2c2a188708ec/packages/flutter_tools/lib/src/vmservice.dart#L383-L390

VmService. _processResponse always interprets an error in the json reponse map as an RPCError

sdk/pkg/vm_service/lib/src/vm_service.dart

Lines 2444 to 2445 in 4faf18d

	} else if (json['error'] != null) {
	request.completeError(RPCError.parse(request.method, json['error']));

I'm don't have a clear understanding of how VmServerConnection was intended to be used yet. Do we expect VmServerConnection to be used to create a VmService object like createVmServiceDelegate does? If so, then I guess we can check for SentinelException here

sdk/pkg/vm_service/lib/src/vm_service.dart

Line 1795 in 4faf18d

final error = e is RPCError

and store SentinelExceptions in a way such that they can be detected and reconstructed here

sdk/pkg/vm_service/lib/src/vm_service.dart

Lines 2444 to 2445 in 4faf18d

	} else if (json['error'] != null) {
	request.completeError(RPCError.parse(request.method, json['error']));

But maybe VmServerConnection is not intended to be used to create a VmService object, in which case we need to design a different solution.

[bkonyi]

A VmServerConnection is effectively a wrapper around the streams (e.g., a web socket in most cases) associated with a single VM service connection that handles routing of VM service requests in a VM service implementation (in this case, DWDS, which implements VmServiceInterface internally).

The issue here is that when the client is invoking getIsolate on an isolate that no longer exists, the VmServerConnection first parses the request, determines it needs to invoke getIsolate(...) and then calls the method. getIsolate(...) will throw a SentinelException which is caught and then converted to a kInternalError here, which isn't correct. We should instead be returning a Sentinel response which takes the following form:

{
  "jsonrpc": "2.0",
  "id": 123,
  "data": {
    "type": "Sentinel"
    "kind": "Collected",
    "valueAsString": "Sentinel <collected>"
  }
}

[annagrin]

@bkonyi I think I tried that in my experiments while fixing the flutter test flake, but a if i remember correctly it does not result in throwing exception from the VmServerConnection.getIsolate as the clients expect, instead it seems to just return a sentinel value. I tried throwing a sentinel exception as well but run into some problems converting it to json.

[derekxu16]

Looks like returning a sentinel value as @bkonyi suggested does fix the problem, because VmService will convert it to a SentinelException here

sdk/pkg/vm_service/lib/src/vm_service.dart

Line 2450 in 4faf18d

request.completeError(SentinelException.parse(request.method, result));

[bkonyi]

SentinelException isn't actually defined in the protocol and is something we added to package:vm_service to avoid needing to return dynamic from the RPCs that can get a Sentinel response. As @derekxu16 has mentioned, we explicitly check for a Sentinel response type in package:vm_service which we then convert to a SentinelException that is thrown.

[derekxu16]

Fixed in package:vm_service v11.2.1

https://pub.dev/packages/vm_service/versions/11.2.1