add a flag to ServerCallStreamObserver to make onCompleted() throw StatusRuntimeException if the call was cancelled

[morgwai]

Is your feature request related to a problem?

It is currently a bit cumbersome If a gRPC method needs to attempt to roll back effects of a call when the client cancels and work is not dispatched to other threads:

• responseObserver.onNext(...) will not throw an exception neither in unary methods (they don't throw on cancel by design) nor in server-streaming (due to issue there should be 1 source of truth regarding whether a call is cancelled #8409 ). Furthermore responseObserver.onNext() may never be called by some server-streaming methods that return a stream of length 0 but need to attempt to roll back side-effects of processing client's request nevertheless.
• onCancelHandler cannot be used as it will be called only after the method exits, due to listener's "called by at most 1 thread at a time" contract.

Describe the solution you'd like

draft: morgwai@f26b7db
If the general idea of this feature request is accepted, I will be happy to prepare a proper PR myself starting from the above draft.

Describe alternatives you've considered

Currently in cases like this, the easiest solution is to keep checking responseObserver.isCancelled() before responseObserver.onCompleted() (and in case of server-streaming calls possibly additionally before each responseObserver.onNext() if it's desirable to interrupt processing ASAP). This however is kinda C-style: client cancelling is an exceptional situation, so I think it's cleaner to handle it in a catch block rather and keep the main positive code-path clean.

Context.addListener() can probably be also used, but as the listener will be called by another thread, it will have to set a specially designated volatile/synchronized flag, which the code of the main positive case will need to examine before responseObserver.onCompleted(). Therefore it's not better than the above solution using responseObserver.isCancelled().

Additional context

Even for methods that do dispatch work to other threads, using exception rather than onCancelHandler is a cleaner solution in some cases: as cancellation may occur in the middle of a call, onCancelHandler similarly as a listener set by Context.addListener() described above, often needs to set a specially designated volatile/synchronized flag to stop further processing, that again needs to be checked by the code of the main positive case, while an exception interrupts the main positive code-path with much less hassle.

[sanjaypujare]

@ejona86 I think we should discuss this as well in our next API meeting (similar to what you said in #8409 (comment))

[sanjaypujare]

This was previously discussed in an API review meeting (7/9/2020) and there are related PRs #7173 and #7172 which discuss our thinking around not throwing exceptions in such cases.

[ejona86]

It is currently a bit cumbersome If a gRPC method needs to attempt to roll back effects of a call when the client cancels and work is not dispatched to other threads

I'm concerned by "roll back." If you mean "cancel your processing," that sounds fine. If you mean "revert the processing" in a transactional sense, then the exception doesn't work. Given you are speaking about onCompleted(), the only way to interpret this seems to be wanting to roll back something akin to a transaction.

The exception is not intended to reliably inform you of a cancellation. That simply is not possible for an async API. The only purpose of the exception is to inform an application so the application can stop its processing and save resources.

There is buffering within the library, within the OS, within the network, within the client's OS, and within the client. Flow control exists in each of those parts which could delay the data, potentially for an unbounded amount of time. #5895 would tell you approximately when the OS has accepted the response data. But beyond that it is simply not possible without strong application involvement.

[morgwai]

@ejona86 that's why I wrote "attempt to roll-back". Generally there's no 100% failure-free transaction mechanism: even 2PC has some tiny margin for errors. For majority of common web-APIs, manual intervention of customer-support / user himself / SREs is acceptable and way cheaper to implement given how tiny is the fraction of cases when a user cancels in the last moment, but his cancellation does not reach the server on time to rollback. I've met myself dozens of teams over the years who chose a small manual intervention 2 times a year (having dozens of qps) over implementing some kind of distributed consensus over REST/gRPC/others.

This issue is only about enabling exception-based programming style in which this can be achieved: in addition to existing C-style "manual" checking for exceptional situation using isCancelled(), add a flag to enable object-oriented use of exception mechanism.

[ejona86]

Okay. In that case then the onCancelHandler is your best bet. It is the most reliable notification of cancellation. And again, cancellation can occur after the server calls onCompleted(). If you are waiting to commit the transaction locally when onCompleted() is called, then #5895 would be a better approach. You'd prepare the transaction within the service handler and call streamObserver.onComplete(). If onCancelHandler fires, cancel the transaction. If (TBD) onCompleteHandler fires, commit the transaction.

Wanting an exception when calling onCompleted() is just broken, and has nothing to do with "c-style" or not. Async APIs generally don't throw, because they've not done the operation yet! Normal Java practice for an async API (e.g., one that returns a Future) is to not throw and instead notify of the failure asynchronously (like failing the Future).

[morgwai]

@ejona86 yes, #5895 sounds as the best option :)
Thanks for pointing and explaining! I'll close this one in this case.

[morgwai]

Also, it would probably be helpful, if it was stated clearly in StreamObserver's javadoc, that calls to the an outbound observers are async and that messages may actually be buffered for significant time after the calls were made.

[ejona86]

I sent out #8449 to document "things are async and can take a while." I didn't call out this specific detail about onCompleted(). That can be done as part of #5895.