stub: Clarify flow control javadoc - #12977
Conversation
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
Removing "peer" from those docs is good.
| * <p>Like {@code StreamObserver}, implementations are not required to be thread-safe; if multiple | ||
| * threads will be writing to an instance concurrently, the application must synchronize its calls. | ||
| * | ||
| * <p>On flow control: The {@link #isReady} state of an outbound {@link CallStreamObserver} is |
There was a problem hiding this comment.
CallStreamObserver is always outbound.
What are we trying to fix by deviating from the "excessive buffering" definition of isReady()? I'm not wild by saying the peer receiving messages via onNext, because the peer may not be using Java. I think it is fair to say there are delays. But I mostly worry that all these words will scare someone into thinking it is too hard instead of helping them understand.
There was a problem hiding this comment.
My highest priority is to remove the incorrect javadoc about flow control. I thought this would be a good time to replace it with something correct and comprehensive, as a second priority. Good point that the peer could be written in any language I can fix that.
There was a problem hiding this comment.
OTOH, the javadoc above already makes a java <-> java assumption with statements like "In any call there are logically four {@link StreamObserver} implementations ..."
There was a problem hiding this comment.
To answer your question directly: what I'm trying to fix is that today there's no factual discussion of the consumer side of the flow-controlled stream. isReady()'s current javadoc LGTM and that covers the sender. On the receiving side, I'm deleting the current javadoc's suggestion that request(int) is what causes an excessively buffered producer/peer to become ready again. I want to replace that with correct information. Calling out message consumption as the thing that unblocks the sender is what lets me address the delay, because we now have a cause and effect event to define that delay. I'll try to do this in fewer words.
764a857 to
9933f6a
Compare
Remove vestigial references to credit-based flow control and the suggestion that a request() for messages directly corresponds to the number of messages a peer could subsequently send before its end of the stream went !isReady(). In fact, okhttp, netty and binder transports all use buffers meaning the sender's stream can be isReady() even when the receiver has no outstanding request()s for messages. Furthermore, those buffers are sized in bytes and messages are not all the same size. So consuming an inbound's next message may not cause a non-ready outbound to become ready again. Replace this with a short discussion of what is actually guaranteed by every transport.
9933f6a to
2af4581
Compare
2 participants
Remove vestigial references to credit-based flow control and the
suggestion that a request() for messages directly corresponds to the
number of messages a peer could subsequently send before its end of the
stream went !isReady(). In fact, okhttp, netty and binder transports all
buffer messages meaning the outbound stream can be isReady() even when the
receiver has no outstanding request()s for messages. Furthermore, those
buffers are sized in bytes and not all messages are the same size. So
consuming an inbound's next message may not cause a non-ready outbound
to become ready again.
Replace this with a short discussion of what is actually guaranteed by
every transport.