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.

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

Fix NIO client-streaming race & flaky test #193

Merged
merged 1 commit into from
Oct 11, 2023
Merged

Conversation

rebello95
Copy link
Collaborator

@rebello95 rebello95 commented Oct 10, 2023

The new client_streaming conformance test that was added in #192 turned out to be flaky (failing a couple times out of 100). This only occurred when using the NIO client, and occurred when:

  • Client caller opens a stream
  • Caller immediately sends one or more chunks of data before the connection is actually established by NIO
  • Caller closes the stream and awaits a response

In this case, the stream would correctly send the queued data after the connection was established, but it would never close and would thus never receive a response, causing the test to time out.

This PR resolves this issue by queueing the close action if the connection has not yet been established (in the same way outbound data is queued).

The fix in this PR was validated locally by running the test several hundreds of times without any failures, and it will fix the flakiness seen on main after #192 merged.

@rebello95 rebello95 requested a review from eseay October 10, 2023 17:33
@rebello95 rebello95 changed the title Fix client-streaming race with NIO & flaky test Fix NIO client-streaming race & flaky test Oct 10, 2023
@eseay
Copy link
Contributor

eseay commented Oct 11, 2023

Makes sense!

@rebello95 rebello95 merged commit 029a83a into main Oct 11, 2023
@rebello95 rebello95 deleted the fix-nio-clientstream branch October 11, 2023 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants