Clarify CallStreamObserver's Javadoc

[leventov]

Fixes #6552.

Apart the class-level comment, there are a few unclear places in the documentation to the methods which I don't know how to improve--please make your suggestions.

1. ... without requiring excessive buffering internally ... in isReady() suggests that there may be some internal buffer, in-process, which may have spare space, and then isReady() can return true. However, the documentation setOnReadyHandler() has phrase when peer is ready to receive more messages which doesn't consider the above option.

2. Phrase On server-side it may only be called during the initial call to the application, before the service returns its {@code StreamObserver}. -- cannot make sense of this phrase, because service implementations don't normally return StreamObserver instances. Does it have something to do with ClientStreamingMethod and BidiStreamingMethod, the StreamObservers returned by them?

3. In the documentation for disableAutoInboundFlowControl(), in the first phrase Disables automatic flow control where a token is returned to the peer after a call to the 'inbound' {@link io.grpc.stub.StreamObserver#onNext(Object)} has completed. it's unclear what does the phrase a token is returned to the peer mean, what is the "token" there.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Roman Leventov (cd1c192)

[ran-su]

This 'inbound' StreamObserver concept also exists in server-side. As an example, you can check here.

[leventov]

Thanks @ran-su for pointing to this example. A comment in this example suggests that isReady may turn to false not only when the remote peer (the client) is not able to receive more messages, but also when they have sent us a lot of messages to overflow the receive buffer. Should it be reflected in the Javadoc for CallStreamObserver.isReady() or setOnReadyHandler()? Or was it actually "send buffer", not "receive buffer" which should be mentioned in this comment:

grpc-java/examples/src/main/java/io/grpc/examples/manualflowcontrol/ManualFlowControlServer.java

Lines 92 to 98 in 9e02cf0

	// Signal the sender to send another request. As long as isReady() stays true, the server will keep
	// cycling through the loop of onNext() -> request()...onNext() -> request()... until either the client
	// runs out of messages and ends the loop or the server runs out of receive buffer space.
	//
	// If the server runs out of buffer space, isReady() will turn false. When the receive buffer has
	// sufficiently drained, isReady() will turn true, and the serverCallStreamObserver's onReadyHandler
	// will be called to restart the message pump.

[ran-su]

In here, server is only response for manage its own ability to receive messages. If client-side's send buffer fills up there are other system mechanisms to handle it.

This example is manual flow control example, which may be considered advanced usage of gRPC. IMO, since this is not for all gRPC users, keep all related information in a more detailed example may be better for both user who don't need this feature and for user who actually want this feature.

[leventov]

How a series of onNext() -> request(1) -> onNext() -> request(1) calls may lead to overflow of server's receive buffer?

[ran-su]

In the code I linked, disableAutoInboundFlowControl() is called above which disables the onNext() -> request(1) -> onNext() -> request(1) cycle. In the case of disableAutoInboundFlowControl, it up to the user of gRPC to call request().

[leventov]

I think the comment that I excerpted above talks about manual calls already. serverCallStreamObserver.request(1);, the very next line after that comment. So yes, user calls serverCallStreamObserver.request(1) (strictly once, as ensured in this line:

grpc-java/examples/src/main/java/io/grpc/examples/manualflowcontrol/ManualFlowControlServer.java

Line 61 in 9e02cf0

if (serverCallStreamObserver.isReady() && wasReady.compareAndSet(false, true)) {

Then gRPC runtime calls incoming StreamObserver's onNext(), and so on, in a cycle (as far as I understand). How the incoming server's receive buffer may steadily grow and become overflown in this case?

The comment I excerpted above would make more sense to me if it was talking about server's send buffer, e. g. that gRPC runtime makes the server to throttle by means of setting isReady to false if the server's send buffer is full, i. e. the client doesn't keep up with responses.

[ran-su]

In the example we are demonstrating a correct way of using those features. If the user is not careful, they may do something equal to request(some_very_big_number), in that case the server receive buffer may get full.

[leventov]

@ran-su thanks. I've updated a comment in ManualFlowControlServer.java hopefully to reflect that less ambiguously. I've also removed the usage of AtomicBoolean there because it took me quite some time of attempts to understand why a CAS operation was needed there in onReadyHandler before I realized that was just a coding convenience.

I've also updated the Javadoc for CallStreamObserver, please review.

Questions 1. and 3. from the message above also remain.

[creamsoup]

@ran-su, is this pr ready to be merged?

[ran-su]

After some discussion, it looks like use boolean is safe here. LGTM

[creamsoup]

merged, thanks @leventov!

[leventov]

Thanks for review and merge!

Could you please also kindly review questions 1. and 3. in the top message in this PR?

[creamsoup]

1. ... without requiring excessive buffering internally ... in isReady() suggests that there may be some internal buffer, in-process, which may have spare space, and then isReady() can return true. However, the documentation setOnReadyHandler() has phrase when peer is ready to receive more messages which doesn't consider the above option.

very good catch! in general the doc in ClientCall or ServerCall is more accurate. although, they should agree each other =p So, the explanation in isReady() is correct. the return statement in the setOnReadyHandler() is incorrect and it should be fixed.

3. In the documentation for disableAutoInboundFlowControl(), in the first phrase Disables automatic flow control where a token is returned to the peer after a call to the 'inbound' {@link io.grpc.stub.StreamObserver#onNext(Object)} has completed. it's unclear what does the phrase a token is returned to the peer mean, what is the "token" there.

yes. this is indeed confusing. the disableAutoInboundFlowControl() is disabling per message "token" e.g. request(1). not necessarily means http2 flow control. we are (or more like were) trying to change to disableAutoRequest(). see discussion in #1788. anyways, basically the "token" is number of messages.