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 use of Cronet refcount #12281

Merged
merged 3 commits into from Sep 5, 2017
Merged

Conversation

muxi
Copy link
Member

@muxi muxi commented Aug 24, 2017

It turns out that Cronet transport never used the stream refcount object (passed in by init_stream). When a call is unref'ed, the stream may be destroyed before the corresponding Cronet stream is done with callbacks, thus introducing crash.

This PR refs the refcount object when a stream is initiated at Cronet transport layer. Unref happens when Cronet issues a finishing callback (either on_succeeded, on_failed, or on_canceled). This guarantees that even the call is unref'ed, the stream context is still valid when the finishing callback happens

The second commit polishes the exec_ctx usage in Cronet transport. In particular, when Cronet issues a callback, the execution context is created immediately. The context is flushed when the callback returns. This is the recommended way of using exec_ctx. It also guarantees that there is no recursive exec_ctx init/flush in a single callback's call stack.

@grpc-kokoro
Copy link

[trickle] No significant performance differences

@grpc-kokoro
Copy link

[microbenchmarks] No significant performance differences

@grpc-kokoro
Copy link

[trickle] No significant performance differences

@grpc-kokoro
Copy link

[microbenchmarks] No significant performance differences

@muxi
Copy link
Member Author

muxi commented Sep 5, 2017

Basic Tests Windows - #12391
Basic Tests C & C++ Linux [dbg] - #11745
Basic Tests C & C++ Linux [opt] - #12335
Asan C++ - Timeout. Should not be affected.

@muxi muxi merged commit 38b9a4d into grpc:master Sep 5, 2017
@muxi muxi deleted the fix-cronet-transport-refcount branch September 5, 2017 16:39
@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
@lock lock bot unassigned makdharma Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants