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

Fix WatchDog leak #828

Closed
igorbernstein2 opened this issue Dec 9, 2019 · 3 comments · Fixed by #838
Closed

Fix WatchDog leak #828

igorbernstein2 opened this issue Dec 9, 2019 · 3 comments · Fixed by #838
Assignees
@igorbernstein2
Labels
🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@igorbernstein2
Copy link
Contributor

WatchDog instances don't currently implement BackgroundResource, which means that they never get unscheduled when the client closes
@igorbernstein2 igorbernstein2 self-assigned this Dec 9, 2019
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Dec 10, 2019
@elharo
Copy link
Contributor

elharo commented Dec 12, 2019

Are you suggesting Watcgdog itself should implement BackgroundResource or all implementations of it should?

@igorbernstein2
Copy link
Contributor Author

The rough idea is:

  • Watchdog implements BackgroundResource
  • on shutdown, the Watchdog unschedules itself from the scheduled executor
  • Watchdog provider gets a new method shouldAutoClose()
  • FixedWatchdogProvider returns false for shouldAutoClose()
  • InstantiatingWatchdogProvider returns true for shouldAutoClose()
  • ClientContext checks if the watchdog needs to be autoclosed and adds it to background resources if yes

This is fairly low priority because the executor that the watchdog is registered with does this already. Which should unschedule the watchdog as a side effect, but it would be cleaner to do this explicitly

@elharo
Copy link
Contributor

elharo commented Dec 12, 2019

Sounds reasonable. Just watch out for breaking changes to the API. Also it might help to resolve #829 first.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Dec 14, 2019
rahulKQL added a commit to rahulKQL/gax-java that referenced this issue Jan 3, 2020
@rahulKQL
feat: implemented watchdog as BackgroundResource
50bf647 
This change updates watchdog to BackgroundResource, Also, watchdog unschedules itself explicitly when shutdown.

closes googleapis#828
@rahulKQL rahulKQL mentioned this issue Jan 3, 2020
Merged
igorbernstein2 pushed a commit that referenced this issue Jan 7, 2020
@rahulKQL @igorbernstein2
feat: implemented watchdog as BackgroundResource (#838)
90f3336 
* feat: implemented watchdog as BackgroundResource

This change updates watchdog to BackgroundResource, Also, watchdog unschedules itself explicitly when shutdown.

closes #828

* updated test cases to increase coverage percentage.

* Moved Watchdog scheduling to start() method

* added javadoc for Watchdog.create()
@meltsufin meltsufin mentioned this issue Nov 17, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@igorbernstein2 igorbernstein2

Labels
🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: implemented watchdog as BackgroundResource rahulKQL/gax-java
3 participants
@elharo @igorbernstein2 @yoshi-automation