Skip to content

Delay client async writer starting #1531

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

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Dec 13, 2022

Motivation:

It's possible for async streaming clients to fail and drop messages in some situations. The situation leading to this happens because streaming calls set up state and then write out headers. While setting up state the HTTP/2 stream channel is configured, when it becomes active gRPC calls outs to enable the async writer to start emitting writes. This can happen before the headers are written so if a write is already pending then it can race the headers being written. If the message is written first then the write promise is failed and the message is dropped.

Modifications:

Delay letting the async writer emit writes until the headers have been written.

Result:

Correct ordering is enforced.

Motivation:

It's possible for async streaming clients to fail and drop messages in
some situations. The situation leading to this happens because streaming
calls set up state and then write out headers. While setting up state
the HTTP/2 stream channel is configured, when it becomes active gRPC
calls outs to enable the async writer to start emitting writes. This can
happen before the headers are written so if a write is already pending
then it can race the headers being written. If the message is written
first then the write promise is failed and the message is dropped.

Modifications:

Delay letting the async writer emit writes until the headers have been
written.

Result:

Correct ordering is enforced.
@glbrntt glbrntt requested a review from FranzBusch December 13, 2022 10:31
@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Dec 13, 2022
Copy link
Collaborator

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

Very nice catch! What an annoying 🐛

@glbrntt glbrntt enabled auto-merge (squash) December 13, 2022 10:35
@glbrntt glbrntt merged commit 4312579 into grpc:main Dec 13, 2022
@glbrntt glbrntt deleted the gb-delay-client-on-start branch December 13, 2022 10:45
WendellXY pushed a commit to sundayfun/grpc-swift that referenced this pull request Aug 24, 2023
Motivation:

It's possible for async streaming clients to fail and drop messages in
some situations. The situation leading to this happens because streaming
calls set up state and then write out headers. While setting up state
the HTTP/2 stream channel is configured, when it becomes active gRPC
calls outs to enable the async writer to start emitting writes. This can
happen before the headers are written so if a write is already pending
then it can race the headers being written. If the message is written
first then the write promise is failed and the message is dropped.

Modifications:

Delay letting the async writer emit writes until the headers have been
written.

Result:

Correct ordering is enforced.
pinlin168 pushed a commit to sundayfun/grpc-swift that referenced this pull request Aug 24, 2023
Motivation:

It's possible for async streaming clients to fail and drop messages in
some situations. The situation leading to this happens because streaming
calls set up state and then write out headers. While setting up state
the HTTP/2 stream channel is configured, when it becomes active gRPC
calls outs to enable the async writer to start emitting writes. This can
happen before the headers are written so if a write is already pending
then it can race the headers being written. If the message is written
first then the write promise is failed and the message is dropped.

Modifications:

Delay letting the async writer emit writes until the headers have been
written.

Result:

Correct ordering is enforced.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants