From b9af80828385a13a7554a0c64360aec2ab4304e1 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Tue, 30 Jun 2020 12:52:32 -0700 Subject: [PATCH 1/5] stub: Redefine exception handling for StreamObserver This aligns the documentation with our recommendations expressed elsewhere ("don't throw"). Exceptions are possible, but having a try-catch around each onNext() would be an anti-pattern for user code as it would be guaranteed to be buggy. We would much rather have higher-level exception handling, covering many application statements, call onError() if any of them throw an exception because the application should generally cancel independent of the source of the exception. --- .../java/io/grpc/stub/StreamObserver.java | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/stub/src/main/java/io/grpc/stub/StreamObserver.java b/stub/src/main/java/io/grpc/stub/StreamObserver.java index 92040d9bc58..dd86692eb39 100644 --- a/stub/src/main/java/io/grpc/stub/StreamObserver.java +++ b/stub/src/main/java/io/grpc/stub/StreamObserver.java @@ -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. + * + *

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 { /** @@ -43,9 +47,16 @@ public interface StreamObserver { * 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. * - *

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. + *

Although implementations are expected to not throw exceptions, if an exception occurs the + * caller is encouraged to call {@link #onError} as part of normal application exception handling. + * gRPC will attempt to call {@code onError()} when an application's implementation throws. + * + *

As an exception to the rule and for historical reasons, on server-side, {@code onNext()} may + * 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 */ @@ -54,9 +65,9 @@ public interface StreamObserver { /** * Receives a terminating error from the stream. * - *

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. + *

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

{@code t} should be a {@link io.grpc.StatusException} or {@link * io.grpc.StatusRuntimeException}, but other {@code Throwable} types are possible. Callers should @@ -71,9 +82,9 @@ public interface StreamObserver { /** * Receives a notification of successful stream completion. * - *

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. + *

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(); } From 35fb0983bcab61398c824f80e276eae11a023263 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Tue, 30 Jun 2020 14:20:22 -0700 Subject: [PATCH 2/5] Add missed update --- stub/src/main/java/io/grpc/stub/StreamObserver.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/stub/src/main/java/io/grpc/stub/StreamObserver.java b/stub/src/main/java/io/grpc/stub/StreamObserver.java index dd86692eb39..f108a0510cf 100644 --- a/stub/src/main/java/io/grpc/stub/StreamObserver.java +++ b/stub/src/main/java/io/grpc/stub/StreamObserver.java @@ -49,7 +49,9 @@ public interface StreamObserver { * *

Although implementations are expected to not throw exceptions, if an exception occurs the * caller is encouraged to call {@link #onError} as part of normal application exception handling. - * gRPC will attempt to call {@code onError()} when an application's implementation throws. + * 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. * *

As an exception to the rule and for historical reasons, on server-side, {@code onNext()} may * throw with a {@code StatusRuntimeException} if the client cancels the call after {@code From 5a0b5d697a74def05b8d393f3ec646c58a183c19 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 1 Jul 2020 08:32:04 -0700 Subject: [PATCH 3/5] The exception is independent of whether onCompleted() was already called --- stub/src/main/java/io/grpc/stub/StreamObserver.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/stub/src/main/java/io/grpc/stub/StreamObserver.java b/stub/src/main/java/io/grpc/stub/StreamObserver.java index f108a0510cf..0dd4ff7a0e5 100644 --- a/stub/src/main/java/io/grpc/stub/StreamObserver.java +++ b/stub/src/main/java/io/grpc/stub/StreamObserver.java @@ -54,11 +54,10 @@ public interface StreamObserver { * onError()} when an application's implementation throws. * *

As an exception to the rule and for historical reasons, on server-side, {@code onNext()} may - * 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. + * throw with a {@code StatusRuntimeException} if the call is cancelled. 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 */ From d688676b7b3b1a3de6c316415172c3db023b4fa0 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 1 Oct 2020 13:56:15 -0700 Subject: [PATCH 4/5] Stop talking about exceptions, except the "don't" at the very top --- .../java/io/grpc/stub/StreamObserver.java | 22 +++++-------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/stub/src/main/java/io/grpc/stub/StreamObserver.java b/stub/src/main/java/io/grpc/stub/StreamObserver.java index 0dd4ff7a0e5..bf6c3c61e11 100644 --- a/stub/src/main/java/io/grpc/stub/StreamObserver.java +++ b/stub/src/main/java/io/grpc/stub/StreamObserver.java @@ -47,17 +47,9 @@ public interface StreamObserver { * 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. * - *

Although implementations are expected to not throw exceptions, if an exception occurs the - * caller is encouraged to call {@link #onError} as part of normal application exception handling. - * 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. - * - *

As an exception to the rule and for historical reasons, on server-side, {@code onNext()} may - * throw with a {@code StatusRuntimeException} if the call is cancelled. 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. + *

For historical reasons, on server-side {@code onNext()} may throw {@code + * StatusRuntimeException} if the call is cancelled. Services are encouraged to use {@link + * ServerCallStreamObserver#setOnCancelHandler} which disables this exception-throwing behavior. * * @param value the value passed to the stream */ @@ -66,9 +58,7 @@ public interface StreamObserver { /** * Receives a terminating error from the stream. * - *

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. + *

May only be called once and if called it must be the last method called. * *

{@code t} should be a {@link io.grpc.StatusException} or {@link * io.grpc.StatusRuntimeException}, but other {@code Throwable} types are possible. Callers should @@ -83,9 +73,7 @@ public interface StreamObserver { /** * Receives a notification of successful stream completion. * - *

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. + *

May only be called once and if called it must be the last method called. */ void onCompleted(); } From 4e0ec05498ad032890dbb7ebcbb03ece5839610b Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 1 Oct 2020 14:52:23 -0700 Subject: [PATCH 5/5] Use active voice --- stub/src/main/java/io/grpc/stub/StreamObserver.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/stub/src/main/java/io/grpc/stub/StreamObserver.java b/stub/src/main/java/io/grpc/stub/StreamObserver.java index bf6c3c61e11..4a3070da48d 100644 --- a/stub/src/main/java/io/grpc/stub/StreamObserver.java +++ b/stub/src/main/java/io/grpc/stub/StreamObserver.java @@ -32,9 +32,9 @@ * 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. * - *

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. + *

Implementations should 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 { /**