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

ClientCalls.ThreadlessExecutor should reject Runnables after end of RPC #3557

Closed
ejona86 opened this issue Oct 9, 2017 · 3 comments · Fixed by #7798
Closed

ClientCalls.ThreadlessExecutor should reject Runnables after end of RPC #3557

ejona86 opened this issue Oct 9, 2017 · 3 comments · Fixed by #7798
Milestone

Comments

@ejona86
Copy link
Member

ejona86 commented Oct 9, 2017

As seen in #3537, the executor was being used for multiple RPCs which caused some Runnables to be queued but never run. We are in a position to know they will never execute though, so it'd be nice to have a (possibly unreliable) detection of rejected Runnables to notice issues like this earlier.

This could be accomplished with just a volatile shutdown boolean that is checked before execute() and a shutdown() method. That's racy, but we're just wanting it to detect bugs, not provide correctness.

@ejona86 ejona86 added this to the Next milestone Oct 9, 2017
njhill added a commit to njhill/grpc-java that referenced this issue Jun 9, 2020
As reported in grpc#7105. Not sure if this is how you want it done, but it does fix the problem.

Fixes grpc#7105
Fixes grpc#3557
njhill added a commit to njhill/grpc-java that referenced this issue Jun 9, 2020
As reported in grpc#7105. Not sure if this is how you want it done, but it does fix the problem.

Fixes grpc#7105
Fixes grpc#3557
@ejona86
Copy link
Member Author

ejona86 commented Jun 22, 2020

Note: this is made more complicated by grpc's current handling of application exceptions (centering around closed), as we deliver an application exception immediately back to the application in close(), but the transport trails behind and can continue scheduling work. This was done to provide a clean set of callbacks to the application. It would be possible to cancel the RPC, then wait (by short-circuiting Runnables) for the close() to arrive, although we'd then need to replace the Status with the cancellation (vs using a status possibly from the server, which we can't give to the user, since it is out-of-context if we chose not to deliver messages).

@ejona86
Copy link
Member Author

ejona86 commented Sep 29, 2020

With #7187 and #7457, to my knowledge we are no longer scheduling runnables after onClose().

njhill added a commit to njhill/grpc-java that referenced this issue Jan 11, 2021
njhill added a commit to njhill/grpc-java that referenced this issue Jan 11, 2021
ejona86 pushed a commit that referenced this issue Feb 23, 2021
…of RPC

Changes originally proposed as part of #7106.

Fixes #3557
@ejona86
Copy link
Member Author

ejona86 commented Mar 16, 2021

Reverted by #7920. It found a bug!

@ejona86 ejona86 reopened this Mar 16, 2021
YifeiZhuang pushed a commit to YifeiZhuang/grpc-java that referenced this issue Jan 19, 2022
YifeiZhuang pushed a commit to YifeiZhuang/grpc-java that referenced this issue Mar 8, 2022
YifeiZhuang added a commit that referenced this issue Mar 8, 2022
… disable by default (#8973)

* stub: Have ClientCalls.ThreadlessExecutor reject Runnables after end of RPC

Changes originally proposed as part of #7106.

Fixes #3557

* add environment variable rejectExecutedException

Co-authored-by: Nick Hill <nickhill@us.ibm.com>
@ejona86 ejona86 modified the milestones: Next, v1.45 Mar 30, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
1 participant