Watchdog waits on executor shutdown in awaitTermination, even if ExecutorProvider has shouldAutoClose == false

[johnjcasey]

Environment details

• Programming language:
• OS:
• Language runtime version:
• Package version: 2.19.2

Steps to reproduce

1. When using a BigQueryWriteClient with a custom ExecutorProvider
2. And that executorProvider has shouldAutoClose == false
3. The watchdog that gets spun up in ClientContext uses this provided executor, but is marked as shouldAutoClose == true (which is correct, because the watchdog has futures that need to be closed)
4. This results in the watchdog awaiting on executor shutdown when awaitTermination is called

[blakeli0]

I tested in my local that Watchdog is not shutting down Executor, the implementation of shutdown() in Watchdog can also confirm that it's only cancelling the future, not shutting down Executor.

Based on the linked PR, I guess the problem you are trying to solve is that you are using isShutdown() to determine if the client is shutdown or not, and Watchdog.isShutdown() is always returning false because the Executor will not be shutdown if ExecutorProvider.shouldAutoClose() is false?

[johnjcasey]

We are using awaitTermination at the client level, which causes watchdogs to await, which depends on the await of the underlying executor. Ultimately, if the executor shouldn't be shutdown, I don't think we should await on the executor within the watchdog either

[blakeli0]

Ultimately, if the executor shouldn't be shutdown, I don't think we should await on the executor within the watchdog either

I agree the WatchDog should not wait on the executor to shutdown if the executor is provided by an ExecutorProvider that has shouldAutoClose() set to false, which is a problem that need to be fixed and I successfully reproduced.

But that's a different problem from what you are describing here, based on the title of issue and the comment in the PR, are you seeing the Watchdog try to shutdown the Executor even if ExecutorProvider has shouldAutoClose == false? If yes, this is something I couldn't reproduce, can you provide some code snippets to reproduce it if you can?

[johnjcasey]

yep. When we provide this executor: https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/util/UnboundedScheduledExecutorService.java

and configure the executor provider like this: https://github.com/johnjcasey/beam/blob/feature/21368-re-enable-unboundedScheduledExecutor/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java#L1503

we see the issue when we later call .awaitTermination https://github.com/johnjcasey/beam/blob/feature/21368-re-enable-unboundedScheduledExecutor/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java#L1414

[blakeli0]

I still couldn't reproduce it with UnboundedScheduledExecutorService, Watchdog.shutdownNow only cancels the future and Watchdog.awaitTermination only waits for the executor but does not shut it down directly.
The Executor is still alive after I called

      this.newWriteClient.shutdownNow();
      this.newWriteClient.awaitTermination(60, TimeUnit.SECONDS);
      this.newWriteClient.close();

[johnjcasey]

Are you creating a BQ client? the newWriteClient in the above code is a BigQueryWriteClient, for context.

It wraps a ClientContext, and calls forwards the various shutdown methods to the ClientContext.backgroundResources

[blakeli0]

Yes, I'm creating a BigQueryWriteClient, the various shutdown methods eventually call various shutdown methods in Watchdog, but none of them are shutting down ExecutorService.

[johnjcasey]

Ah! The problem we were seeing is that our await termination is hanging for the duration of the timeout provided (60s), because the underlying executor isn't getting terminated.

[johnjcasey]

but this await termination call shouldn't wait for the executor to be terminated

[johnjcasey]

Sorry I've been explaining some of the issues poorly

[blakeli0]

@johnjcasey No worries, can you please update the title and description to match the issue you were observing? I'll prepare a fix in the mean time.