Skip to content
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions stub/src/main/java/io/grpc/stub/StreamObserver.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@
* not need to be synchronized together; incoming and outgoing directions are independent.
* Since individual {@code StreamObserver}s are not thread-safe, if multiple threads will be
* writing to a {@code StreamObserver} concurrently, the application must synchronize calls.
*
* <p>Implementations are expected to not throw exceptions, except when called incorrectly. For
* example, a call to {@code onNext()} after {@code onCompleted()} may throw. {@link Error}s may
* also still occur.
*/
public interface StreamObserver<V> {
/**
Expand All @@ -43,9 +47,18 @@ public interface StreamObserver<V> {
* server streaming calls, but may receive many onNext callbacks. Servers may invoke onNext at
* most once for client streaming calls, but may receive many onNext callbacks.
*
* <p>If an exception is thrown by an implementation the caller is expected to terminate the
* stream by calling {@link #onError(Throwable)} with the caught exception prior to
* propagating it.
* <p>Although implementations are expected to not throw exceptions, if an exception occurs the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you avoid the passive voice and just say "implementations should not throw" (here and in 3 other places)?

* caller is encouraged to call {@link #onError} as part of normal application exception handling.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a subtle change to the contract, specifically that callers are no longer expected to necessarily pass the caught exception itself to onError?

* That is to say, {@code onError} should be called if any related part of the application throws
* an exception, generally independent of the source, to avoid leaks. gRPC will call {@code
* onError()} when an application's implementation throws.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is helpful but perhaps it should go in the javadoc for the whole interface since it isn't specific to onNext? You could say:

Owners of a StreamObserver are responsible for eventually calling either onError() or onCompleted() to ensure that resources associated with the stream (memory, file descriptors, etc) are released.

*
* <p>As an exception to the rule and for historical reasons, on server-side, {@code onNext()} may
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here {@code onNext()} is about the response stream observer, and {@code onCompleted()} at the end of this sentence is about the request stream observer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I realize that is wrong. It is unconditional. Fixed, which side-steps the confusing language.

* throw with a {@code StatusRuntimeException} if the client cancels the call after {@code
* onCompleted()}. This was necessary because there was not a callback available to notify the
* service. Services are encouraged to use {@link
* ServerCallStreamObserver#setOnCancelHandler} which provides a callback and disables this
* exception-throwing behavior.
*
* @param value the value passed to the stream
*/
Expand All @@ -54,9 +67,9 @@ public interface StreamObserver<V> {
/**
* Receives a terminating error from the stream.
*
* <p>May only be called once and if called it must be the last method called. In particular if an
* exception is thrown by an implementation of {@code onError} no further calls to any method are
* allowed.
* <p>May only be called once and if called it must be the last method called. Although
* implementations are expected to not throw exceptions, if an exception is thrown by {@code
* onError()} further calls to the observer should be avoided.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saying "further calls should be avoided" sounds a bit odd/redundant to me coming immediately after "must be the last method called" (same for onCompleted())

*
* <p>{@code t} should be a {@link io.grpc.StatusException} or {@link
* io.grpc.StatusRuntimeException}, but other {@code Throwable} types are possible. Callers should
Expand All @@ -71,9 +84,9 @@ public interface StreamObserver<V> {
/**
* Receives a notification of successful stream completion.
*
* <p>May only be called once and if called it must be the last method called. In particular if an
* exception is thrown by an implementation of {@code onCompleted} no further calls to any method
* are allowed.
* <p>May only be called once and if called it must be the last method called. Although
* implementations are expected to not throw exceptions, if an exception is thrown by {@code
* onCompleted()} further calls to the observer should be avoided.
*/
void onCompleted();
}