The Ready event is missed in RetriableStream. Sublistener::onReady is called but masterListener.onReady() is not.

[bjxutj]

What version of gRPC-Java are you using?

I am using 1.26.0, and I have tried 1.27.2 and latest code 1.27.3 (compiled by myself).

What is your environment?

Windows 10, with openjdk 11.0.2

What did you expect to see?

I expect to see the onReady is called, then trigger masterListener.onReady().

What did you see instead?

onReady is called, but masterListener.onReady() is not called.

Steps to reproduce the bug

Sorry, it is hard to reproduce, but I can analyze the code to explain.
In drain(Substream) of RetriableStream, the new state was generated when all the entries were runWith the substream. but in the start(), a StartEntry was added, then the stream started after runWith. There is a possibility that the stream has sent the data successfully and call onReady to notify (another thread), but the new state which include the substream in the drainedSubstreams was not generated, then masterListener.onReady() was not called.

[sanjaypujare]

@bjxutj since you are saying it's hard to reproduce I am assuming there is no immediate impact or not looking for an urgent fix. If you have any code snippet/example to share that will be good.

[sanjaypujare]

@dapengzhang0 might have some comments...

[dapengzhang0]

I can agree the current implementation (Sublistener.onReady()) does not handle onReady correctly in all cases, as the comment says

https://.com/grpc/grpc-java/blob/v1.26.0/core/src/main/java/io/grpc/internal/RetriableStream.java#L934-L939

@bjxutj Is this causing failure for your application or any performance problem?

[bjxutj]

@dapengzhang0 Yes, in our test, some times the request was blocked because it didn't receive onReady event. I want to know if we have a solution to avoid this problem?

[bjxutj]

@dapengzhang0 I am confusing on Sublistener.onReady(), does it really need to check if (state.drainedSubstreams.contains(substream)) ? In which case, we should check this? Can we just pass through the onReady event to the master listener, they will handle if the stream is cancelled or closed, or retry ...

[dapengzhang0]

@dapengzhang0 I am confusing on Sublistener.onReady(), does it really need to check if (state.drainedSubstreams.contains(substream)) ? In which case, we should check this? Can we just pass through the onReady event to the master listener, they will handle if the stream is cancelled or closed, or retry ...

@bjxutj Are you just doing the normal retry (without hedging)? If that's the case, calling master listener is fare, but it will break hedging more. In the hedging case, we just know that onReady() has already broken concurrency. So we might be okay with that in short term. We just want to make sure that will fix your problem.

[bjxutj]

No, in fact, I have switched to no-retry stream, because we don't need the retry mechanism. When we use this retriable stream, in my test, it didn't retry, just normal case, maybe it is too fast or too slow, some thread problem to cause this issue. Thanks. Regards ZHANG Dapeng <notifications@.com> 于2020年6月2日周二 下午2:37写道：

…

@dapengzhang0 <https://.com/dapengzhang0> I am confusing on Sublistener.onReady(), does it really need to check if (state.drainedSubstreams.contains(substream)) ? In which case, we should check this? Can we just pass through the onReady event to the master listener, they will handle if the stream is cancelled or closed, or retry ... @bjxutj <https://.com/bjxutj> Are you just doing the normal retry (without hedging)? If that's the case, calling master listener is fare, but it will break hedging more. In the hedging case, we just know that onReady() has already broken concurrency. So we might be okay with that in short term. We just want to make sure that will fix your problem. — You are receiving this because you were mentioned. Reply to this email directly, view it on <#6817 (comment)>, or unsubscribe <https://.com/notifications/unsubscribe-auth/AE57BZ27GYFFZ3UWGPOJNCDRUVBFDANCNFSM4LF2EI4A> .