Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

Fix Race condition when shutting down executor/transport channel #787

Merged
merged 1 commit into from Sep 24, 2019

Conversation

vam-google
Copy link
Contributor

This fixes the customer issue ("transitively"): #785

This PR only changes the order of shutdowns (first transportChannel and only then the Executor). This the most lightweight fix we can make, which should fix the issue. We would like to avoid enforcing awaitTermination() on shutdown() because that would convert non-blocking call (shutdown()) to a blocking one with great potential of having non-desirable side-effects. Also this may potentially require surface changes on gax (potentially requiring major version bump on gax), because currently shutdown() is widely used, it is declared in a public interface and is explicitly specified as non-blocking operation.

This fix only changes the order of shutdowns (first transportChannel and only then the Executor). This the most lightweight fix we can make, which should fix the issue. We would like to avoid enforcing `awaitTermination()` on `shutdown()` because that would convert non-blocking call (`shutdown()`) to a blocking one with great potential of having non-desirable side-effects. Also this may potentially require surface changes on gax (potentially requiring major version bump on gax), because currently `shutdown()` is widely used, it is declared in a public interface and is explicitly specified as non-blocking operation.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 5, 2019
@codecov
Copy link

codecov bot commented Sep 5, 2019

Codecov Report

Merging #787 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #787   +/-   ##
=========================================
  Coverage     78.39%   78.39%           
  Complexity     1106     1106           
=========================================
  Files           198      198           
  Lines          4887     4887           
  Branches        385      385           
=========================================
  Hits           3831     3831           
  Misses          887      887           
  Partials        169      169
Impacted Files Coverage Δ Complexity Δ
...ain/java/com/google/api/gax/rpc/ClientContext.java 81.81% <100%> (ø) 8 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4aec0f...9d86f11. Read the comment docs.

@vam-google
Copy link
Contributor Author

@chingor13 PTAL

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants