stub: Wait for onClose when blocking stub is interrupted #5579
Conversation
Interceptors need to see the onClose to clean up properly. This also changes an isInterrupted() to interrupted(), since previously the interrupted flag was still set when InterruptedException was thrown. This caused an infinite loop with the new code. Previously, all callers immediately re-set the interrupted flag, so there was no issue. Fixes grpc#5576
ejona86
added
the
TODO:backport
| .withDescription("Call was interrupted") | ||
| .withCause(e) | ||
| .asRuntimeException(); | ||
| interrupt = true; |
There was a problem hiding this comment.
optional:
...
int interrupt = 0;
if (interrupts++ == 1) {
throw Status.....asRuntimeException();
}
This allows a single interrupt to clean up, but a second interrupt while waiting for onClose to bypass.
There was a problem hiding this comment.
Cancelling multiple times is a noop. I'm not too interested in optimizing this, especially since multiple interrupts would be unlikely.
There was a problem hiding this comment.
It's not so much an optimization, but a safety check. If the onClose callback gets dropped it would allow this thread to escape and shutdown.
There was a problem hiding this comment.
We allow an infinite number of interrupts, waiting for the onClose. I'm not sure how the onClose could be dropped here.
There was a problem hiding this comment.
We discussed offline, and the thought is this could block for a long time waiting for the onClose. If interrupted twice we could choose to exit early, without waiting for the onClose.
I think this is more appropriate at the moment, although I agree it is unfortunate we have to wait for the onClose. If we had #636 done and a fallback executor, we could add a shutdown to ThreadlessExecutor where it would begin throwing RejectedExecutionException on execute(). We would then shutdown() when we exit the loop here. We can then return more quickly, knowing that if/when the onClose is scheduled it will still be run (on the fallback executor).
| .withDescription("Call was interrupted") | ||
| .withCause(e) | ||
| .asRuntimeException(); | ||
| interrupt = true; |
There was a problem hiding this comment.
Cancelling multiple times is a noop. I'm not too interested in optimizing this, especially since multiple interrupts would be unlikely.
ejona86
removed
the
TODO:backport
Interceptors need to see the onClose to clean up properly.
This also changes an isInterrupted() to interrupted(), since previously
the interrupted flag was still set when InterruptedException was thrown.
This caused an infinite loop with the new code. Previously, all callers
immediately re-set the interrupted flag, so there was no issue.
Fixes #5576
CC @ravirajj
CC @njhill, since I touched the ThreadlessExecutor because of isInterrupted()