googleapis / gax-java

This library has moved to https://github.com/googleapis/sdk-platform-java/tree/main/gax-java.
https://github.com/googleapis/gapic-generator-java/tree/main/gax-java
BSD 3-Clause "New" or "Revised" License
162 stars 119 forks source link

fix: [Approach 1]Watchdog does not shut down executor on client closing if the executor is provided by ExecutorProvider. #1875

Closed blakeli0 closed 1 year ago

blakeli0 commented 2 years ago

This PR is an attempt to fix https://github.com/googleapis/gax-java/issues/1858. Note that the issue above is just the behavior of multiple convoluted issues. The background is as below:

  1. Watchdog implemented BackgroundResources in https://github.com/googleapis/gax-java/pull/838
  2. To fix https://github.com/googleapis/gax-java/issues/870, Watchdog does not shutdown executor anymore in shutdown https://github.com/googleapis/gax-java/pull/871
  3. Watchdog does not shutdown executor anymore in shutdownNow https://github.com/googleapis/gax-java/pull/1158
  4. Watchdog.isShutdown still checks if the executor is shutdown, hence hanging forever if the executor is provided by ExecutorProvider https://github.com/googleapis/gax-java/issues/1858

In short, the root cause is that Watchdog.shutdown and Watchdog.isShutdown should have consistent behavior, but we also need to decide if we want Watchdog to manage the Executor or not.

There are three possible approaches:

  1. This PR. Watchdog still manages its own executor on shutdown and shouldAutoClose is the source of truth to decide if we want to shutdown executor or not, have to expose a new interface(not breaking since we can add it as a default) in WatchdogProvider to set shouldAutoClose. This is my preferred approach since I think it's the cleanest one and still provides the ability to auto close the executors provided to Watchdog.
  2. https://github.com/googleapis/gax-java/pull/1883. Watchdog still manages its own executor on shutdown, but we use both shouldAutoClose and needExecutor to decide if we want to shutdown executor or not.
  3. https://github.com/googleapis/gax-java/pull/1884. Watchdog does not manage its own executor on shutdown, only cancels futures.
sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

91.7% 91.7% Coverage
0.0% 0.0% Duplication

blakeli0 commented 1 year ago

Closing in favor of https://github.com/googleapis/gax-java/pull/1890 and https://github.com/googleapis/gax-java/pull/1884