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

[chttp2] fix stream leak with queued flow control update and absence of writes #30907

Merged
merged 58 commits into from Sep 14, 2022

Conversation

apolcyn
Copy link
Contributor

@apolcyn apolcyn commented Sep 9, 2022

As discovered with internal issue b/242712384, there's a leak that can happen as follows:

  1. Client RPC closes writes, e.g. calls WritesDone()
  2. Client attempts to read a message and trailers, and receives trailers only.

Since writes are closed first, chttp2 will call grpc_chttp2_mark_stream_closed with close_reads=1, during 2). This can cause remove_stream to be called before updating flow control and taking a new ref on the stream.

In my repro of b/242712384, I observed this last stream flow control update to always have queuing urgency. In this case, nothing is guaranteed to initiate further writes on the transport (and the RPC has completed as far as the application is concerned). So even after the application destroys the channel and fully unrefs the RPC, the ref held by the queued flow control update will keep the stream and connection objects alive indefinitely.


Edit:

Since #30550 submitted, the above description doesn't actually repro because of the change to StreamFlowControl::UpdateAction. But the same underlying bug can also repro in a different way, which is what the test is now based off of:

  1. server sends a message and status

  2. client polls the connection, reads message and status off the wire and marks the stream closed, without having yet started a RECV_MESSAGE or RECV_STATUS op

  3. client performs RECV_MESSAGE and RECV_STATUS ops, which complete immediately. But the stream flow control update is queued and not flushed out because we already had all the data and didn't need to send any pre-emptive updates.

@apolcyn apolcyn marked this pull request as ready for review September 9, 2022 18:17
@apolcyn apolcyn merged commit d8f98fb into grpc:master Sep 14, 2022
ctiller added a commit that referenced this pull request Sep 14, 2022
ctiller added a commit that referenced this pull request Sep 14, 2022
apolcyn added a commit to apolcyn/grpc that referenced this pull request Sep 14, 2022
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Sep 14, 2022
apolcyn added a commit that referenced this pull request Sep 15, 2022
…ate and absence of writes (#30907)" (#30991)" (#30992)

* properly synchronize grpc_iomgr_count_objects_for_testing

* Revert "Revert "[chttp2] fix stream leak with queued flow control update and absence of writes (#30907)" (#30991)"

This reverts commit 0f2a0f5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants