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.

Sign up for GitHub

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

Jump to bottom

Clarify CallStreamObserver's Javadoc #6561

Merged
merged 3 commits into from Jan 28, 2020
Merged

Clarify CallStreamObserver's Javadoc #6561

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cd1c192
Clarify CallStreamObserver's Javadoc
leventov Dec 20, 2019
7a2dab2
Remove unnecessary AtomicBoolean and clarify a comment in ManualFlowC…
leventov Dec 29, 2019
73ccaab
Correct Javadoc of CallStreamObserver
leventov Dec 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 13 additions & 8 deletions stub/src/main/java/io/grpc/stub/CallStreamObserver.java
View file
Open in desktop
Expand Up @@ -19,20 +19,25 @@
import io.grpc.ExperimentalApi;

/**
* A refinement of StreamObserver provided by the GRPC runtime to the application that allows for
* more complex interactions with call behavior.
* A refinement of StreamObserver provided by the GRPC runtime to the application (the client or
* the server) that allows for more complex interactions with call behavior.
*
* <p>In any call there are logically two {@link StreamObserver} implementations:
* <p>In any call there are logically three {@link StreamObserver} implementations:
* <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

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

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.

Copy link
Member

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

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

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 •
edited

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.

Copy link
Member

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

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.

* is implemented by the client application.
* </li>
* <li>'outbound' - which the GRPC runtime provides to the application which it uses to
* send messages to the remote peer.
* <li>'outbound', client-side - which the GRPC runtime provides to the client application and the
* client uses this {@code StreamObserver} to send messages to the server.
* </li>
* <li>'outbound', server-side - which the GRPC runtime provides to the server application and
* the server uses this {@code StreamObserver} to send messages (responses) to the client.
* </li>
* </ul>
*
* <p>Implementations of this class represent the 'outbound' message stream.
* <p>Implementations of this class represent the 'outbound' message streams. The client-side
* one is {@link ClientCallStreamObserver} and the service-side one is
* {@link ServerCallStreamObserver}.
*
* <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.
Expand Down