Fix hot restart flake

[annagrin]

Fixes latest flakes in web hot restart tests.

Details

I was able to add delays in various parts of the code to repro the flake pretty consistently - it was caused by a race between serving DevTools and hot restart with a following interleaving sequence of events:

• the call to

  flutter/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart

  Line 593 in 961df98

  unawaited(residentDevtoolsHandler!.serveAndAnnounceDevTools(

  got the flutter views with the old isolate id in FlutterVmService.findExtensionIsolate
• hot restart killed the isolate
• chrome proxy service returned a sentinel which missed the check in

  flutter/packages/flutter_tools/lib/src/vmservice.dart

  Line 1022 in 961df98

  (error is vm_service.RPCError && error.code == RPCErrorCodes.kServiceDisappeared)) {

  end errored instead in

  flutter/packages/flutter_tools/lib/src/vmservice.dart

  Line 1025 in 961df98

  return Future<vm_service.Isolate?>.error(error, stackTrace);

The fix

Update the check to account for sentinel exceptions returned from VmServerConnection in https://github.com/dart-lang/sdk/blob/67f3e1f4a0dfe57c07192607095892a9d107a9d1/pkg/vm_service/lib/src/vm_service.dart#L1795

Closes: #121708

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[annagrin]

@bkonyi chrome proxy service throws a sentinel exception on getIsolate for non-existing isolate id, as required by the VmServiceInterface, 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, so I have to check for the string contents here. Is there a better way to handle this error? Should we change the VMServer connection to throw something different, or have a set of error codes in package:vm_service we all agree on and can check for here?

[bkonyi]

service.getIsolate(...) should be throwing a SentinelException in this case, so there's likely something wrong with the encoding on the DWDS side of things. What's the JSON response you're seeing here? It should look something like:

{
  "id": "123",
  "result": {
    "type": "Sentinel",
    "kind": "Collected",
    "valueAsString": "..."
  }
}

[bkonyi]

If I had to guess, the "error" property is being populated instead, which is incorrect.

[annagrin]

dwds's Chrome proxy service throws a sentinel exception, but the vm server connection we use on top of it converts it to RPC error with code of RPCError.kInternalError here:
https://github.com/dart-lang/sdk/blob/67f3e1f4a0dfe57c07192607095892a9d107a9d1/pkg/vm_service/lib/src/vm_service.dart#L1795

Should this code be changed then?

[annagrin]

I will file an issue in the SDK repo for this, in the meanwhile the current PR should fix the flake.

[annagrin]

Filed an issue (i am worried about all the other cases where we miss error checks due to this problem): dart-lang/sdk#51752

[christopherfujino]

LGTM

[christopherfujino]

Landing on red luci-flutter as this fixes one of the reasons the tree is currently red

[ditman]

Wowowow, thanks for the fix! 🙏