core: DelayedStream should start() real stream immediately#7750
Merged
ejona86 merged 3 commits intogrpc:masterfrom Jan 20, 2021
Merged
core: DelayedStream should start() real stream immediately#7750ejona86 merged 3 commits intogrpc:masterfrom
ejona86 merged 3 commits intogrpc:masterfrom
Conversation
DelayedClientTransport needs to avoid becoming terminated while it owns RPCs. Previously DelayedClientTransport could terminate when some of its RPCs had their realStream but realStream.start() hadn't yet been called. To avoid that, we now make sure to call realStream.start() synchronously with setting realStream. Since start() and the method calls before start execute quickly, we can run it in-line. But it does mean we now need to split the Stream methods into "before start" and "after start" categories for queuing. Fixes grpc#6283
Member
Author
|
A test will be added as part of #7749 |
Member
|
We went through the flow, and here is an over simplified chart to illustrate why RPCs will never be orphaned after the fix. FYI @dapengzhang0 |
dapengzhang0
approved these changes
Jan 7, 2021
Contributor
dapengzhang0
left a comment
dapengzhang0
left a comment
There was a problem hiding this comment.
LGTM. A couple of minor comments.
YifeiZhuang
reviewed
Jan 7, 2021
YifeiZhuang
approved these changes
Jan 20, 2021
YifeiZhuang
added a commit
that referenced
this pull request
Jan 29, 2021
Add more delayedStream tests related to #7750, where we changed to call realStream.start() synchronously with setting realStream.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DelayedClientTransport needs to avoid becoming terminated while it owns
RPCs. Previously DelayedClientTransport could terminate when some of its
RPCs had their realStream but realStream.start() hadn't yet been called.
To avoid that, we now make sure to call realStream.start()
synchronously with setting realStream. Since start() and the method
calls before start execute quickly, we can run it in-line. But it does
mean we now need to split the Stream methods into "before start" and
"after start" categories for queuing.
Fixes #6283
CC @voidzcy