fix: Watchdog controls lifecycle of the future, not executor #1890
Conversation
| } | ||
|
|
||
| @Override | ||
| public boolean isTerminated() { | ||
| return executor.isTerminated(); | ||
| return future.isDone(); |
There was a problem hiding this comment.
Probably equivalent to future.isCancelled().
There was a problem hiding this comment.
I think isDone and isCancelled are equivalent. And both are slightly wrong for isTerminated. I think the strict definition of isTerminated is: not only has the task been unscheduled, but also that the last invocation stopped running.
Practically I dont think it matters too much. So if you want to take a shortcut and just use isDone, but document it.
Alternatively use a CountdownLatch where each run() is wrapped in an increment & decrement operation on the latch. awaitTermination can then block on the latch
There was a problem hiding this comment.
From the Javadoc for isDone:
Returns true if this task completed. Completion may be due to normal termination, an exception, or cancellation -- in all of these cases, this method will return true.
It sounds like the last invocation would have stopped running. What makes you think otherwise?
There was a problem hiding this comment.
I think the cancellation of the future is only cancelling the scheduled tasks, if the run() method is already running and takes long time to complete, then isDone() could return true while the run() is still running. I just reproduced this in my local as well.
That being said, not sure if it matters that much in practice like Igor said, I don't know how long it takes for a typical run() to complete. Using a CountdownLatch is not enough in this case since it does not support increment, only decrement. So we may need something more if we want to fix it perfectly.
|
Kudos, SonarCloud Quality Gate passed! |
🤖 I have created a release *beep* *boop* --- ## [2.20.1](https://togithub.com/googleapis/gax-java/compare/v2.20.0...v2.20.1) (2022-12-02) ### Bug Fixes * **deps:** Update dependency com.google.api.grpc:grpc-google-common-protos to v2.11.0 ([#1906](https://togithub.com/googleapis/gax-java/issues/1906)) ([d27d848](https://togithub.com/googleapis/gax-java/commit/d27d8485d3da4de00253c1f5df435516d1af8d8e)) * **deps:** Update dependency com.google.api.grpc:proto-google-common-protos to v2.11.0 ([#1907](https://togithub.com/googleapis/gax-java/issues/1907)) ([7504e37](https://togithub.com/googleapis/gax-java/commit/7504e37163d39d10bd8388101e9ce614e2839aca)) * **deps:** Update dependency com.google.cloud:google-cloud-shared-config to v1.5.5 ([#1911](https://togithub.com/googleapis/gax-java/issues/1911)) ([772c221](https://togithub.com/googleapis/gax-java/commit/772c2213dfa10120b6efccf411550e77df3f9de7)) * **deps:** Update dependency com.google.protobuf:protobuf-bom to v3.21.10 ([#1912](https://togithub.com/googleapis/gax-java/issues/1912)) ([f508f24](https://togithub.com/googleapis/gax-java/commit/f508f245a59b2086c4b56d55f0cb25e9e7c29136)) * Watchdog controls lifecycle of the future, not executor ([#1890](https://togithub.com/googleapis/gax-java/issues/1890)) ([bd1714e](https://togithub.com/googleapis/gax-java/commit/bd1714e484eef8aa8f09632eee976c9da26da5f1)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Watchdog should not control the lifecycle of the provided executor. As a
BackgroundResource, it should only unschedule itself from the executor if asked to be shutdown.I believe this is more aligned with the intent #828 without surprising side effects on the lifecycle of the executor that is not controlled by the
Watchdogor theWatchdogProvider.Fixes #1858.
cc/ @igorbernstein2
Other approaches considered: #1875 , #1883.
Will add unit tests in a separate PR.
Further improvement: #1884.