Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Failures on Update to latest package:json_rpc_2 #43012

Closed
zichangg opened this issue Aug 10, 2020 · 4 comments · Fixed by dart-archive/json_rpc_2#65
Closed

Failures on Update to latest package:json_rpc_2 #43012

zichangg opened this issue Aug 10, 2020 · 4 comments · Fixed by dart-archive/json_rpc_2#65
Assignees
@natebosch
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. gardening

Comments

@zichangg
Copy link
Contributor

There are new test failures on Update to latest package:json_rpc_2.

The tests

service/external_service_notification_invocation_test/dds RuntimeError (expected Pass)

are failing on configurations

dartkp-win-release-x64

log: https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8872333091123864224/+/steps/test_results/0/logs/new_test_failures__logs_/0
@a-siva a-siva added the area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. label Aug 10, 2020
@natebosch
Copy link
Member

This appears flaky, the same test is passing in other configurations.

The error does look plausibly related to my changes in that package, but without being able to reproduce this locally I'm not quite sure.

Should I roll this back in the mean time?

natebosch added a commit to dart-archive/json_rpc_2 that referenced this issue Aug 11, 2020
@natebosch
Avoid an open Peer with a closed Client
8b9b2f1 
May fix dart-lang/sdk#43012

If a `Peer` is created with a `StreamChannel` that does not follow the
stated contract it's possible that the `sink` gets closed without
receiving a done event from the `channel` which leaves the `Peer`
instance in a state that's inconsistent with the underlying `Client`.
The result is that it's possible to get a bad state trying to send a
message even with `isClosed` returns `false`.

- Make `isClosed` and `done` forward to the `_client` and `_peer` fields
  so that they can't be inconsistent.
- Forward errors to the `_server` so that it can forward them through
  `done` without an extra `Completer` to manage.
- Avoid closing the `sink` in the `Peer`. It will end up being closed by
  the server when it is handling the error, and it's the same `sink`
  instance in both places.
- Add a test that ensures that `isClosed` behaves as expected following
  a call to `close()` even when the `StreamChannel` does not follow it's
  contract.
@natebosch natebosch mentioned this issue Aug 11, 2020
Merged
natebosch added a commit to dart-archive/json_rpc_2 that referenced this issue Aug 11, 2020
@natebosch
Avoid an open Peer with a closed Client (#65)
995611c 
May fix dart-lang/sdk#43012

If a `Peer` is created with a `StreamChannel` that does not follow the
stated contract it's possible that the `sink` gets closed without
receiving a done event from the `channel` which leaves the `Peer`
instance in a state that's inconsistent with the underlying `Client`.
The result is that it's possible to get a bad state trying to send a
message even with `isClosed` returns `false`.

- Make `isClosed` and `done` forward to the `_client` and `_peer` fields
  so that they can't be inconsistent.
- Forward errors to the `_server` so that it can forward them through
  `done` without an extra `Completer` to manage.
- Avoid closing the `sink` in the `Peer`. It will end up being closed by
  the server when it is handling the error, and it's the same `sink`
  instance in both places.
- Add a test that ensures that `isClosed` behaves as expected following
  a call to `close()` even when the `StreamChannel` does not follow it's
  contract.
@natebosch natebosch reopened this Aug 11, 2020
@natebosch
Copy link
Member

I'm not sure how to proceed with this. I spent some time trying to see if I could reproduce a similar failure outside of the SDK and I was able to track down at least one way to cause the behavior I saw in the logs. That was fixed with dart-archive/json_rpc_2#65

When trying to sync that it surfaced a different bug in dds that was being masked by bugs in json_rpc_2. I fixed that bug in https://dart-review.googlesource.com/c/sdk/+/158220

But now I'm stuck. When I try to sync today I can get a green run on this bot, but it's not clear to me if that is sufficient.

Here is a green run: https://ci.chromium.org/p/dart/builders/try/vm-kernel-precomp-win-release-x64-try/1872?

There are some things here I don't understand at all.

  • The "Test Results" link shows: 
    dartkp-win-release-x64
Pass -> RuntimeError (expected Pass)
service/external_service_disappear_test/dds
  • The "Test Results" link does not show a message about service/external_service_notification_invocation_test/dds going from RuntimeError to Pass
  • The results.json link shows both service/external_service_disappear_test/dds and service/external_service_notification_invocation_test/dds as "Pass".

One thing that is interesting, flaky.json does contain service/external_service_notification_invocation_test/dds.

I have been completely unable to see this failure locally.

So I have some questions:

@whesse

  • Why is there a discrepancy between results.json and https://dart-ci.firebaseapp.com/cl/158206/3 ? Is it showing me the failure still from a previous run?
  • How should I interpret this try result? Is it safe to submit my CL? Would submitting that be considered enough to close this issue?
  • How can I see the output from the run(s) that failed?
  • Is there any way for me to more narrowly check a patch to see if it fixes a given test?

@bkonyi - can you help me dig here? I think you might own some of these tests?

  • Do you know if this test has been flaky in the past?
  • Do you know why this failure might only show up on windows? Are you able to run this test locally and see an error?

@bkonyi
Copy link
Contributor

bkonyi commented Aug 12, 2020

@bkonyi - can you help me dig here? I think you might own some of these tests?

  • Do you know if this test has been flaky in the past?
  • Do you know why this failure might only show up on windows? Are you able to run this test locally and see an error?

Service tests are notoriously flaky on Windows. This is an older test that's being run against DDS, so it's possible that the extra latency introduced by communicating with the service via DDS has made the test more flaky. It's also hard to tell what the actual failure is caused by since we check the error code before we check the message so the error message is never written to the console.

I think you're safe to land, but if you go ahead and swap lines 84 and 85 in runtime/observatory/tests/service/external_service_disappear_test.dart we'll get a more useful error output if we hit it again.

@natebosch
Copy link
Member

@bkonyi - The test I'm most interested in currently is service/external_service_notification_invocation_test/dds

This is the failure that I am intending to address with dart-archive/json_rpc_2#65.

The stack trace was:

  Bad state: The client is closed.

  package:json_rpc_2/src/client.dart 152                      Client._send
  package:json_rpc_2/src/client.dart 140                      Client.sendNotification
  package:json_rpc_2/src/peer.dart 102                        Peer.sendNotification
  package:dds/src/client.dart 74                              _DartDevelopmentServiceClient.sendNotification

DDS is trying to do the right thing here and check whether the peer is closed, but a bug in json_rpc_2 meant that a Peer could appear open, but it's Client was closed and so the call fails.

sdk/pkg/dds/lib/src/client.dart

Lines 71 to 74 in 3b6aeb4

if (_clientPeer.isClosed) {
return;
}
_clientPeer.sendNotification(method, parameters);

I was able to track down one way that can lead to this situation and I fixed that. (Side note, my repro required a StreamChannel that does not fit the contract around close guarantees which suggests that either DDS or maybe just that test has a misbehaving StreamChannel). Since I can't see the logs and I can't reproduce a failure locally I don't know if they flakiness in this test is still the same thing (and I haven't fixed it), or something unrelated...

The other test service/external_service_disappear_test/dds was fixed ahead of time by https://dart-review.googlesource.com/c/sdk/+/158220 and I'm hoping @whesse will confirm that there is something going wrong in the UI that it still showing it as a new failure.

dart-bot pushed a commit that referenced this issue Aug 17, 2020
@natebosch
Update to latest json_rpc_2
efab30a 
Contains a potential fix for a bug causing #43012.

Change-Id: I861ca6dbb10398edb29181fb8a1348db73818f95
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/158206
Commit-Queue: Nate Bosch <nbosch@google.com>
Auto-Submit: Nate Bosch <nbosch@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@natebosch natebosch

Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. gardening
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Avoid an open Peer with a closed Client dart-archive/json_rpc_2
4 participants
@natebosch @a-siva @bkonyi @zichangg