Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ClientCall errors does not finally invoke ClientCall.Listener.onClose() #5576

Closed
ravirajj opened this issue Apr 10, 2019 · 2 comments · Fixed by #5579 or #6255
Closed

ClientCall errors does not finally invoke ClientCall.Listener.onClose() #5576

ravirajj opened this issue Apr 10, 2019 · 2 comments · Fixed by #5579 or #6255
Assignees
Labels
Milestone

Comments

@ravirajj
Copy link

ravirajj commented Apr 10, 2019

Please answer these questions before submitting your issue.

What version of gRPC are you using?

v1.19.0

What did you expect to see?

ClientCall.Listener.onClose() should be invoked when there is a ClientCall error for unary calls.

When there is an exception thrown by call.sendMessage() or call.halfClose() within asyncUnaryRequestCall() (See https://sourcegraph.com/github.com/grpc/grpc-java/-/blob/stub/src/main/java/io/grpc/stub/ClientCalls.java#L276:37), it results in cancelThrow() calling ClientCall.cancel() and scheduling a stream closed task (i.e. ClientCall.Listener.onClose) on the executor. However, the exception propagates up to futureUnaryCall and blockingUnaryCall which catches it again, calls cancelThrow() again but never drains the executor within the try-catch block (See https://sourcegraph.com/github.com/grpc/grpc-java/-/blob/stub/src/main/java/io/grpc/stub/ClientCalls.java#L123-146). Thus ClientCall.cancel() is called but not ClientCall.Listener.onClose() on client call errors.

Is ClientCall.Listener.onClose() expected to be called whenever the client call terminates? Is dropping the ClientCall.Listener.onClose() call a bug or expected behaviour?

@ejona86 ejona86 self-assigned this Apr 10, 2019
@ejona86 ejona86 added this to the Next milestone Apr 10, 2019
@ejona86 ejona86 added the bug label Apr 10, 2019
ejona86 added a commit to ejona86/grpc-java that referenced this issue Apr 10, 2019
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 a commit that referenced this issue Apr 22, 2019
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
@ejona86 ejona86 reopened this Apr 26, 2019
@ejona86
Copy link
Member

ejona86 commented Apr 26, 2019

Fix was reverted in #5641

@ejona86
Copy link
Member

ejona86 commented Sep 17, 2019

See cl/245431681 for information about the failing test.

@ejona86 ejona86 modified the milestones: Next, 1.25 Oct 10, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.