stub: Redefine exception handling for StreamObserver #7172
Conversation
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.
| * an exception, generally independent of the source, to avoid leaks. gRPC will call {@code | ||
| * onError()} when an application's implementation throws. | ||
| * | ||
| * <p>As an exception to the rule and for historical reasons, on server-side, {@code onNext()} may |
There was a problem hiding this comment.
Here {@code onNext()} is about the response stream observer, and {@code onCompleted()} at the end of this sentence is about the request stream observer?
There was a problem hiding this comment.
Actually, I realize that is wrong. It is unconditional. Fixed, which side-steps the confusing language.
| * <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 |
There was a problem hiding this comment.
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. | ||
| * 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. |
There was a problem hiding this comment.
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 | ||
| * 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 |
There was a problem hiding this comment.
Consider omitting "This was necessary because ... " since teaching callers the actual historical reason doesn't seem worth the extra reading
| * 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. |
There was a problem hiding this comment.
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())
| * 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 | ||
| * caller is encouraged to call {@link #onError} as part of normal application exception handling. |
There was a problem hiding this comment.
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?
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.