stub,examples: fix comments related to ServerCallStreamObserver.isReady() to clarify it applies to sending side readiness #8910
Conversation
…dy() to clarify it applies to sending side readiness
| // | ||
| // Note: the onReadyHandler's invocation is serialized on the same thread pool as the incoming StreamObserver's | ||
| // onNext(), onError(), and onComplete() handlers. Blocking the onReadyHandler will prevent additional messages | ||
| // from being processed by the incoming StreamObserver. The onReadyHandler must return in a timely manner or | ||
| // from being processed by the sending StreamObserver. The onReadyHandler must return in a timely manner or |
There was a problem hiding this comment.
Revert this. It was previously correct, and the change is incorrect. The inbound stream observer is the one with callbacks, and those are sharing the executor.
There was a problem hiding this comment.
Okay, I think the main thing to note is "onReadyHandler's invocation is serialized on the same thread pool as the incoming StreamObserver's onNext(), onError(), and onComplete() handlers" and with that the "incoming" makes sense.
I was thinking that the isReady() call is on the sending side and hence the onReadyHandler is also on the sending side i.e. the callbacks are made from the sending side so if you block inside the callback you are blocking the sending side's processing.
Will revert ...
| @@ -42,11 +42,11 @@ public static void main(String[] args) throws InterruptedException, IOException | |||
| serverCallStreamObserver.disableAutoRequest(); | |||
|
|
|||
| // Set up a back-pressure-aware consumer for the request stream. The onReadyHandler will be invoked | |||
| // when the consuming side has enough buffer space to receive more messages. | |||
| // when the sending side has enough buffer space to store more responses. | |||
There was a problem hiding this comment.
"consuming side" makes sense to talk about. But "sending side" is this side, so is strange. Maybe instead:
The onReadyHandler will be invoked when there is available buffer space to store more responses.
There was a problem hiding this comment.
Back-pressure detection includes both local and remote buffer availability on the sending side, is that right? If the remote side (client in this case) has no buffer to receive responses it will cause back-pressure (thru TCP) and the sending side's buffer will fill up. And when that buffer is full, that is when isReady() will be false, IIUC.
Your suggested wording is concise and better so I will change to that.
| @@ -98,7 +98,7 @@ public void disableAutoRequest() { | |||
|
|
|||
|
|
|||
| /** | |||
| * If {@code true}, indicates that the observer is capable of sending additional messages | |||
| * If {@code true}, indicates that the observer is capable of sending additional responses | |||
There was a problem hiding this comment.
Meh. Does this actually help anything?
There was a problem hiding this comment.
The reason I wanted to clarify this is because of the confusing and relative nature of words like "receiving" and "sending" in this context. For example, if you are a server you are receiving request messages on the wire through the requestObserver. But the requestObserver is "sending" these messages to the application through the onNext callback and I think some docs/comments do have that usage which is why I wanted to disambiguate here.
fixed examples/manualflowcontrol/ManualFlowControlServer.java
fixed stub/ServerCallStreamObserver.java
to clarify the wording
@ejona86 as discussed.