Skip to content
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

Merged
merged 3 commits into from Jan 28, 2020

Conversation

leventov
Copy link
Contributor

@leventov leventov commented Dec 20, 2019

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
Copy link

@linux-foundation-easycla linux-foundation-easycla bot commented Dec 20, 2019

CLA Check
The committers are authorized under a signed CLA.

* <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
Copy link
Member

@ran-su ran-su Dec 20, 2019

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.

Copy link
Contributor Author

@leventov leventov Dec 22, 2019

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:

// 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.

Copy link
Member

@ran-su ran-su Dec 23, 2019

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.

Copy link
Contributor Author

@leventov leventov Dec 23, 2019

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?

Copy link
Member

@ran-su ran-su Dec 23, 2019

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().

Copy link
Contributor Author

@leventov leventov Dec 24, 2019

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:

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.

Copy link
Member

@ran-su ran-su Dec 26, 2019

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.

Copy link
Contributor Author

@leventov leventov Dec 29, 2019

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.

@creamsoup
Copy link
Contributor

@creamsoup creamsoup commented Jan 24, 2020

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

@creamsoup creamsoup added the kokoro:run label Jan 28, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run label Jan 28, 2020
ran-su
ran-su approved these changes Jan 28, 2020
Copy link
Member

@ran-su ran-su left a comment

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

@ran-su ran-su requested a review from creamsoup Jan 28, 2020
@creamsoup creamsoup merged commit 589a645 into grpc:master Jan 28, 2020
13 checks passed
@creamsoup
Copy link
Contributor

@creamsoup creamsoup commented Jan 28, 2020

merged, thanks @leventov!

@leventov leventov deleted the CallStreamObserver-javadoc branch Jan 29, 2020
@leventov
Copy link
Contributor Author

@leventov leventov commented Jan 29, 2020

Thanks for review and merge!

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

@creamsoup
Copy link
Contributor

@creamsoup creamsoup commented Jan 29, 2020

  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.

  1. 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.

@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants