Skip to content

Conversation

@annagrin
Copy link
Contributor

@annagrin annagrin commented Mar 16, 2023

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 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 flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 16, 2023
@flutter-dashboard
Copy link

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.

error is vm_service.RPCError &&
(error.code == RPCErrorCodes.kServiceDisappeared ||
error.code == RPCErrorCodes.kInternalError &&
error.message.contains('Sentinel kind: Collected'))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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": "..."
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@annagrin annagrin Mar 16, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

@christopherfujino
Copy link
Contributor

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

@ditman
Copy link
Member

ditman commented Mar 17, 2023

Wowowow, thanks for the fix! 🙏

christopherfujino pushed a commit to chris-forks/flutter that referenced this pull request Mar 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mac web_tool_tests is 5.05% flaky

4 participants