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
Clarify CallStreamObserver's Javadoc #6561
Conversation
|
* <ul> | ||
* <li>'inbound' - which the GRPC runtime calls when it receives messages from the | ||
* remote peer. This is implemented by the application. | ||
* <li>'inbound' - which the GRPC runtime calls when it receives messages from the server. This |
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.
This 'inbound' StreamObserver
concept also exists in server-side. As an example, you can check here.
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.
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. |
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.
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.
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.
How a series of onNext() -> request(1) -> onNext() -> request(1) calls may lead to overflow of server's receive buffer?
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.
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()
.
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.
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.
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.
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.
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.
@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.
@ran-su, is this pr ready to be merged? |
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.
After some discussion, it looks like use boolean is safe here. LGTM
merged, thanks @leventov! |
Thanks for review and merge! Could you please also kindly review questions 1. and 3. in the top message in this PR? |
very good catch! in general the doc in
yes. this is indeed confusing. the |
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.
... without requiring excessive buffering internally ...
inisReady()
suggests that there may be some internal buffer, in-process, which may have spare space, and thenisReady()
can return true. However, the documentationsetOnReadyHandler()
has phrasewhen peer is ready to receive more messages
which doesn't consider the above option.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 returnStreamObserver
instances. Does it have something to do withClientStreamingMethod
andBidiStreamingMethod
, theStreamObserver
s returned by them?In the documentation for
disableAutoInboundFlowControl()
, in the first phraseDisables 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 phrasea token is returned to the peer
mean, what is the "token" there.