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

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

Closed
wants to merge 3 commits into from

Conversation

blakeli0
Copy link
Contributor

@blakeli0 blakeli0 commented Nov 14, 2022

This PR is an attempt to fix #1858.
Note that the issue above is just the behavior of multiple convoluted issues. The background is as below:

  1. Watchdog implemented BackgroundResources in feat: implemented watchdog as BackgroundResource #838
  2. To fix WatchDog shuts down ExecutorService from FixedExecutorProvider #870, Watchdog does not shutdown executor anymore in shutdown fix: do not shut down executor when WatchDog is shut down #871
  3. Watchdog does not shutdown executor anymore in shutdownNow fix: Watchdog.shutdownNow() does not shutdown executor #1158
  4. Watchdog.isShutdown still checks if the executor is shutdown, hence hanging forever if the executor is provided by ExecutorProvider Watchdog waits on executor shutdown in awaitTermination, even if ExecutorProvider has shouldAutoClose == false #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. fix: [Approach 2] Watchdog does not shut down executor on client closing if the executor is provided by ExecutorProvider. #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. fix: Watchdog does not does not wait for executor to shutdown on awaitTermination #1884. Watchdog does not manage its own executor on shutdown, only cancels futures.

@blakeli0 blakeli0 changed the title fix: Watchdog does not shut down executor on client closing if the executor is provided by ExecutorProvider. fix: [Approach 1]Watchdog does not shut down executor on client closing if the executor is provided by ExecutorProvider. Nov 15, 2022
@sonarcloud
Copy link

sonarcloud bot commented Nov 15, 2022

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
Copy link
Contributor Author

blakeli0 commented Dec 8, 2022

Closing in favor of #1890 and #1884

@blakeli0 blakeli0 closed this Dec 8, 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