there should be 1 source of truth regarding whether a call is cancelled

[morgwai]

What version of gRPC-Java are you using?

1.39.0

What is your environment?

openJdk-11, ubuntu

What did you expect to see?

StatusRuntimeException should be thrown consistently by responseObserver.onNext(...) if responseObserver.isCancelled() == true

What did you see instead?

unless a server dispatches work to other threads, an exception is not thrown by responseObserver.onNext(...) regardless of responseObserver.isCancelled() == true.
This is due to the fact that there are 2 competing sources on whether a call was cancelled:

• responseObserver.isCancelled() calls call.isCancelled()
• responseObserver has its own additional cancelled flag that is checked by onNext(...). This flag is set by listener.onCancel() and as listener can be called by at most 1 thread concurrently, the flag is not set until a user method exits.

It seems that observer's additional cancelled flag does not bring any value and is completely redundant: I think it should be removed and instead responseObserver.onNext() should be checking cancellation status directly by call.isCancelled() the same way responseObserver.isCancelled() does.

Steps to reproduce the bug

see https://groups.google.com/g/grpc-io/c/4g9XpeqNngE/m/T_ZqBlWeAQAJ

[morgwai]

#8411 removes the redundant flag. All tests pass and the example from the linked group post works as expected when using the patched version.

[ejona86]

I think you misunderstand the purpose of onNext() throwing. Ideally, onNext() would not throw. The only reason it throws is because after the inbound StreamObserver.onCompleted() is called there was no way to be notified of cancellation. This was resolved by adding the setOnCancelHandler() API.

Throwing from onNext() based on cancellation is a poor multi-threaded API because the server has no way to prevent the exception (other than on-cancel handler); the onNext() method begins throwing as soon as cancellation is noticed and the server may be sending a message in another thread at that precise time.

We would strongly recommend using setOnCancelHandler(), which disables the throwing behavior.

[morgwai]

We would strongly recommend using setOnCancelHandler(), which disables the throwing behavior.

@ejona86
I agree. Nevertheless, when the handler is unset, the code should behave consistently.

Please have a look at the PR: the cancelled var of the ServerCallStreamObserverImpl is completely redundant: removing it simplifies the code and make it behave consistently.

[morgwai]

the onNext() method begins throwing as soon as cancellation is noticed

@ejona86 this statement is currently inaccurate: this is exactly what this bug is about.
The example I posted to mailing list and linked in the initial message of this issue, demonstrates a pretty common case when responseObserver.isCancelled() returns true, but responseObserver.onNext(...) in the next line does not throw exception although onCancelHandler is not set.

[ejona86]

the onNext() method begins throwing as soon as cancellation is noticed

@ejona86 this part is incorrect: this is exactly what this bug is about.

I was purposefully vague when I said "noticed." It depends on your perspective. gRPC noticed the cancellation, but the application hadn't been informed yet via onCancel(). In your case blocking within the onNext() callback is preventing the cancellation notification.

The current behavior is very old. It has the advantage that the application will not be informed of additional messages via onNext() after it sees the cancellation exception. With the suggested change of relying on isCancelled(), other callbacks can still happen which might confuse code.

That said, looking at your code organization, I see the advantage of having onNext() begin throwing ASAP, since you probably have a loop sending many messages. Having a isCancelled() check would be very easy if you think about it.

Given the age of this behavior, I'm hesitant to change it even if it produces better results. There is certainly code that has never seen the exception and would begin seeing it with the change... and probably break. But I do see the strong value to new code; cancellation is important. I'll bring this up in an API review meeting on Thursday.

To be honest, myself and others have never liked the throwing behavior but it was required. As a general rule we would strongly encourage using setOnCancelHandler() instead of relying on the exception. But I do see how the exception is convenient in your code.

demonstrates a pretty common case when responseObserver.isCancelled() returns true, but responseObserver.onNext(...) in the next line does not throw exception although onCancelHandler is not set.

I'm not too concerned about that case, because if you are actually calling isCancelled() manually and still chose to send a message via onNext() then it seems your code doesn't actually care about cancellation. We could fix this pretty easily by setting cancelled = isCancelled() within responseObserver.isCancelled(); that'd be a pretty safe change in my mind although it is strange that calling isCancelled() has side-effects. That probably doesn't help your true goals though.

[morgwai]

I'm not too concerned about that case, because if you are actually calling isCancelled() manually and still chose to send a message via onNext()

@ejona86
please let me emphasize that a call responseObserver.isCancelled() in the linked example is only to prove that the gRPC system already does have a knowledge that a call was cancelled prior to responseObserver.onNext(...) call (that does not throw regardless). As a matter of fact, I don't recall using isCancelled() ever before ;-)
By "pretty common case" I meant a situation where a code iterates over some response-producing resource (like a DB query result cursor) right in a given gRPC method, without dispatching to other threads.

I usually use onCancelHandler just as you recommend, but sometimes it's more convenient to rely on the exception: onCancelHandler is called kinda out of context of the "main code" handling requests (on another thread, I mean), so onCancelHandler usually needs to set some specially designated flag (volatile or synchronized), that the main code must examine in its main loop. When all you need is just for the main code to break from the loop, the exception is visibly less hassle.

Given the age of this behavior, I'm hesitant to change it even if it produces better results. There is certainly code that has never seen the exception and would begin seeing it with the change... and probably break.

that's a fair point.

But I do see the strong value to new code; cancellation is important. I'll bring this up in an API review meeting on Thursday.

great, thanks!

[ejona86]

As a matter of fact, I don't recall using isCancelled() ever before ;-)

Yeah, I assumed as much. I just called that out because you argued in part that the exception wasn't matching the value of isCancelled(). That can be argued from an API-only perspective or from a pragmatic perspective. I was mostly arguing that it doesn't matter too much to me from a legalese standpoint, as no code should really notice the disagreement and it is easy to workaround. It seemed you were caring about this pragmatically, so it was more "let's not get sidetracked on the legalistic argument."

(Other APIs I would care more about the legalistic argument. But this particular one is quite old and an API wart to begin with.)

When all you need is just for the main code to break from the loop, the exception is visibly less hassle.

Totally agree that onCancelHandler takes plumbing. For your code organization approach I'd probably set a no-op Runnable as the onCancelHandler to disable the exception and then check isCancelled() as part of the loop. That's about as much boilerplate as dealing with the exception. Today you wouldn't need the no-op Runnable; after your suggested change you would.

The main reason I'd want to cause onNext() to throw sooner is so that code that didn't think about cancellation would abort. The runtime exception has its own problems, but I imagine many times it is worked out early in testing/deployment.

[morgwai]

@ejona86
one more point to consider when making decision about this issue and the linked PR:
in cases that do care about interrupting processing as soon as possible (for example to release some limited resources, like pooled DB connections), when work is done without dispatching to other threads, using onCancelHandler is not an option as it would not get called until the "main code" exits due to listener's "1 concurrent thread" contract.
Therefore, in such cases the available options are the exception or manually checking isCancelled() in the main loop as you mentioned. Both get the work done with similar amount of code, however I think that manually checking isCancelled() is kinda C-style coding: 1 of the main reasons to have exceptions in a language is not to "pollute" the main positive-case code with checks for all kind of errors and exceptional situations.

Nevertheless, i do acknowledge your previous point, that changing the behavior now may break things that were working seamlessly for long time :/ So if you choose to reject my PR because of this, I will just silently cry in some corner where nobody can see, he he ;-)

[ejona86]

in cases that do care about interrupting processing as soon as possible

In those cases, use Context.addListener().

I will note that your code only looks so clean in dealing with the exception because it isn't observing outbound flow control. To do that you'd need to actually be async to receive the onReady() callback. Not to say flow control actually benefits your code; if you have small CPU-intensive responses it doesn't do much. My point is just that the situation is more specific than it may appear.

I really wish we had a blocking streaming API to make this sort of thing easier.

[ejona86]

API review meeting notes (from Friday, 8/20/2021):

• Votes (all happen to be forms of "don't make change"):
  • +1 Don’t encourage the throwable usage
  • +3 Too dangerous to change
  • +1 Ideally would fix it. But hard to absorb behavior change
• Javadoc for onNext throwing is confusing. It should maybe be updated
• Should add a comment to the code saying we are keeping previous behavior to avoid throwing in new situations

[ejona86]

To provide some color to "too dangerous to change," basically you can take the code sample for this issue as an example of code that is likely to be broken by the change. Any code that was blocking to send replies on a streaming RPC (the very case this wants to improve) would never have seen an exception before.

[morgwai]

[silent cry from some dark corner]
;-)