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

Race condition when shutting down executor/transport channel #785

Closed
chingor13 opened this issue Sep 4, 2019 · 1 comment
Closed

Race condition when shutting down executor/transport channel #785

chingor13 opened this issue Sep 4, 2019 · 1 comment
Assignees
@vam-google
Labels
priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@chingor13
Copy link
Contributor

ClientContext collects background resources to close after the ClientContext is closed:

  • ExecutorService (ExecutorAsBackgroundResource)
  • TransportChannel

When GrpcOperationsStub#close() is called, it calls shutdown() on an aggregation of the ExecutorService and TransportChannel which calls shutdown() on these resources in order. Since shutdown does not block and initiates a graceful shutdown, it's possible that the ExecutorService is terminated before the TransportChannel finishes (race condition).

We should probably awaitTermination on the TransportChannel before trying to shutdown the executor.

See googleapis/google-cloud-java#5810
@chingor13 chingor13 added the type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. label Sep 4, 2019
@vam-google vam-google self-assigned this Sep 4, 2019
@vam-google vam-google added the priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. label Sep 4, 2019
@vam-google vam-google mentioned this issue Sep 5, 2019
Merged
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Sep 9, 2019
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Sep 19, 2019
vam-google added a commit that referenced this issue Sep 24, 2019
@vam-google
Fix Race condition when shutting down executor/transport channel (#787)
2dfae40 
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.
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Sep 24, 2019
@vam-google
Copy link
Contributor

Should be fixed by #787.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@vam-google vam-google

Labels
priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@chingor13 @vam-google @yoshi-automation