-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
core: fix retry flow control issue #8401
core: fix retry flow control issue #8401
Conversation
047dfed
to
e43b829
Compare
@@ -64,6 +64,7 @@ | |||
|
|||
private final MethodDescriptor<ReqT, ?> method; | |||
private final Executor callExecutor; | |||
private final Executor listenerSerializeExecutor = new SerializeReentrantCallsDirectExecutor(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SerializeReentrantCallsDirectExecutor is single-producer (not thread safe). It doesn't work in this case. I think SynchronizationContext is probably better suited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There has been an issue about flow control when retry is enabled. Currently we call `masterListener.onReady()` whenever `substreamListener.onReady()` is called. The user's `onReady()` implementation might do ``` while(observer.isReady()) { // send one more message. } ``` However, currently if the `RetriableStream` is still draining, `isReady()` is false, and user's `onReady()` exits immediately. And because `substreamListener.onReady()` is already called, it may not be called again after drained. This PR fixes the issue by - Use a SerializeExecutor to call all `masterListener` callbacks. - Once `RetriableStream` is drained, check `isReady()` and if so call `onReady()`. - Once `substreamListener.onReady()` is called, check `isReady()` and only if so we call `masterListener.onReady()`.
There has been an issue about flow control when retry is enabled. Currently we call `masterListener.onReady()` whenever `substreamListener.onReady()` is called. The user's `onReady()` implementation might do ``` while(observer.isReady()) { // send one more message. } ``` However, currently if the `RetriableStream` is still draining, `isReady()` is false, and user's `onReady()` exits immediately. And because `substreamListener.onReady()` is already called, it may not be called again after drained. This PR fixes the issue by - Use a SerializeExecutor to call all `masterListener` callbacks. - Once `RetriableStream` is drained, check `isReady()` and if so call `onReady()`. - Once `substreamListener.onReady()` is called, check `isReady()` and only if so we call `masterListener.onReady()`.
There has been an issue about flow control when retry is enabled.
Currently we call
masterListener.onReady()
wheneversubstreamListener.onReady()
is called.The user's
onReady()
implementation might doHowever, currently if the
RetriableStream
is still draining,isReady()
is false, and user'sonReady()
exits immediately. And becausesubstreamListener.onReady()
is already called, it may not be called again after drained.This PR fixes the issue by
masterListener
callbacks.RetriableStream
is drained, checkisReady()
and if so callonReady()
.substreamListener.onReady()
is called, checkisReady()
and only if so we callmasterListener.onReady()
.