fix: Cancel the Timeout Task for HttpJson #2360

lqiu96 · 2024-01-12T17:51:39Z

Fixes: googleapis/google-cloud-java#10220

Currently, the executorService will wait for any previously submitted task to finish execution before being able to terminate. This PR attempts to fix the issue by cancelling the outstanding timeout task. If a response has been received prior to the timeout, the timeout task will be cancelled and the client should be able to terminate immediately afterwards.

This fixes an issue for Clients that have RPCs with a large timeout value (i.e. 10 min). The client would wait 10 minutes before being able to terminate completely.

Local Testing

Running this with SNAPSHOT Gax version (2.41.1-SNAPSHOT) and latest Compute from Libraries-Bom v26.31.0
Logs:

}

	at com.google.api.client.http.HttpResponseException$Builder.build(HttpResponseException.java:293)
	at com.google.api.client.http.HttpRequest.execute(HttpRequest.java:1118)
	at com.google.api.gax.httpjson.HttpRequestRunnable.run(HttpRequestRunnable.java:115)
	... 6 more

Process finished with exit code 1

Running this with latest Gax version (fix not included) and latest Compute from Libraries-Bom v26.31.0:

  }
}

	at com.google.api.client.http.HttpResponseException$Builder.build(HttpResponseException.java:293)
	at com.google.api.client.http.HttpRequest.execute(HttpRequest.java:1118)
	at com.google.api.gax.httpjson.HttpRequestRunnable.run(HttpRequestRunnable.java:115)
	... 6 more

Missing the termination and exit code. It shows up after 10 minutes.

suztomo · 2024-01-12T19:00:44Z

Would you try to express the problem googleapis/google-cloud-java#10220 in a test? The test should fail before your fix.

If it's difficult to write the test, would you describe the challenge in the pull request description?

lqiu96 · 2024-01-16T15:37:21Z

Would you try to express the problem googleapis/google-cloud-java#10220 in a test? The test should fail before your fix.

If it's difficult to write the test, would you describe the challenge in the pull request description?

Will do, thanks! I think there should be a way to test this behavior.

lqiu96 · 2024-01-17T18:16:08Z

Added ITClientShutdown showcase tests to test that the client is terminating properly. I expect that the showcase tests will fail if it's using the HttpJsonClientCallsImpl in the main branch.

Showcase (Java 11): https://github.com/googleapis/sdk-platform-java/actions/runs/7560014731/job/20585113208?pr=2360
Showcase (Java 17): https://github.com/googleapis/sdk-platform-java/actions/runs/7560014731/job/20585114645?pr=2360

lqiu96 · 2024-01-17T18:21:24Z

Error:

Error:  Errors: 
Error:    ITClientShutdown.testGrpc_rpcInvokedWithLargeTimeout_closeClientOnceResponseReceived:110 » TestTimedOut test timed out after 15000 milliseconds
Error:    ITClientShutdown.testHttpJson_rpcInvokedWithLargeTimeout_closeClientOnceResponseReceived:157 » TestTimedOut test timed out after 15000 milliseconds
[INFO] 
Error:  Tests run: 76, Failures: 0, Errors: 2, Skipped: 1

The testGrpc_rpcInvokedWithLargeTimeout_closeClientOnceResponseReceived test should not be failing. It looks like it's because this test is created with HttpJson client. Will update and re-run.

lqiu96 · 2024-01-17T18:23:26Z

Showcase (Java 11): https://github.com/googleapis/sdk-platform-java/actions/runs/7560111797/job/20585419373?pr=2360
Showcase (Java 17): https://github.com/googleapis/sdk-platform-java/actions/runs/7560111797/job/20585419851?pr=2360

I expect both these tests to fail and that testHttpJson_rpcInvokedWithLargeTimeout_closeClientOnceResponseReceived is the only test case that fails.

This reverts commit e6f7219.

lqiu96 · 2024-01-29T21:51:27Z

Took a look at grpc-java's implementation and they keep track of the ScheduledFuture as well.

Interestingly, grpc-java does not interrupt if the timeout future if it is already running: https://github.com/grpc/grpc-java/blob/7f4c16e068eeb6c7049b7ab2f0420a72d2cc1fb2/core/src/main/java/io/grpc/internal/ClientCallImpl.java#L383.

From the showcase tests, grpc's client isn't blocked and is able to terminate. It seems to be since that client's HttpJson's stub manages that deadlineexecutorservice while that the same deadlineexecutorservice is not managed by client's gRPC's stub (rather managed in grpc-java).

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITClientShutdown.java

suztomo · 2024-02-08T14:45:32Z

It seems that you were able to reproduce the problem in googleapis/google-cloud-java#10220. In the pull request description, would you add observation where you confirmed the problem of java-compute is fixed?

gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCallImpl.java

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITClientShutdown.java

lqiu96 · 2024-02-09T18:38:04Z

@suztomo and @burkedavison Could I get a re-review and an approval from both of you if everything looks good?

suztomo

LGTM

sonarcloud · 2024-02-13T20:04:08Z

Quality Gate failed for 'gapic-generator-java-root'

Failed conditions
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

sonarcloud · 2024-02-13T20:07:08Z

Quality Gate failed for 'java_showcase_integration_tests'

Failed conditions
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

🤖 I have created a release *beep* *boop* --- <details><summary>2.35.0</summary> ## [2.35.0](v2.34.0...v2.35.0) (2024-02-13) ### Features * add `generate_repo.py` ([#2431](#2431)) ([47b632a](47b632a)) * move synthtool templates to `library_generation/owlbot` ([#2443](#2443)) ([5c95472](5c95472)) ### Bug Fixes * Apiary Host returns user set host if set ([#2455](#2455)) ([5f17e62](5f17e62)) * Cancel the Timeout Task for HttpJson ([#2360](#2360)) ([b177d9e](b177d9e)) ### Dependencies * update dependency commons-codec:commons-codec to v1.16.1 ([#2473](#2473)) ([8c6e91d](8c6e91d)) * update google api dependencies ([#2469](#2469)) ([ad4d4e6](ad4d4e6)) * update google auth library dependencies to v1.23.0 ([#2466](#2466)) ([349a5d3](349a5d3)) * update google auth library dependencies to v1.23.0 ([#2476](#2476)) ([6c9127c](6c9127c)) * update google http client dependencies to v1.44.1 ([#2467](#2467)) ([87d1435](87d1435)) * update googleapis/java-cloud-bom digest to ac9893c ([#2472](#2472)) ([7fff34a](7fff34a)) * update grpc dependencies to v1.61.1 ([#2463](#2463)) ([9ec575b](9ec575b)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

Fixes: googleapis/google-cloud-java#10220 Currently, the executorService will wait for any previously submitted task to finish execution before being able to terminate. This PR attempts to fix the issue by cancelling the outstanding timeout task. If a response has been received prior to the timeout, the timeout task will be cancelled and the client should be able to terminate immediately afterwards. This fixes an issue for Clients that have RPCs with a large timeout value (i.e. 10 min). The client would wait 10 minutes before being able to terminate completely. ## Local Testing Running this with SNAPSHOT Gax version (2.41.1-SNAPSHOT) and latest Compute from Libraries-Bom v26.31.0 Logs: ``` } at com.google.api.client.http.HttpResponseException$Builder.build(HttpResponseException.java:293) at com.google.api.client.http.HttpRequest.execute(HttpRequest.java:1118) at com.google.api.gax.httpjson.HttpRequestRunnable.run(HttpRequestRunnable.java:115) ... 6 more Process finished with exit code 1 ``` Running this with latest Gax version (fix not included) and latest Compute from Libraries-Bom v26.31.0: ``` } } at com.google.api.client.http.HttpResponseException$Builder.build(HttpResponseException.java:293) at com.google.api.client.http.HttpRequest.execute(HttpRequest.java:1118) at com.google.api.gax.httpjson.HttpRequestRunnable.run(HttpRequestRunnable.java:115) ... 6 more ``` Missing the termination and exit code. It shows up after 10 minutes.

🤖 I have created a release *beep* *boop* --- <details><summary>2.35.0</summary> ## [2.35.0](v2.34.0...v2.35.0) (2024-02-13) ### Features * add `generate_repo.py` ([#2431](#2431)) ([47b632a](47b632a)) * move synthtool templates to `library_generation/owlbot` ([#2443](#2443)) ([5c95472](5c95472)) ### Bug Fixes * Apiary Host returns user set host if set ([#2455](#2455)) ([5f17e62](5f17e62)) * Cancel the Timeout Task for HttpJson ([#2360](#2360)) ([b177d9e](b177d9e)) ### Dependencies * update dependency commons-codec:commons-codec to v1.16.1 ([#2473](#2473)) ([8c6e91d](8c6e91d)) * update google api dependencies ([#2469](#2469)) ([ad4d4e6](ad4d4e6)) * update google auth library dependencies to v1.23.0 ([#2466](#2466)) ([349a5d3](349a5d3)) * update google auth library dependencies to v1.23.0 ([#2476](#2476)) ([6c9127c](6c9127c)) * update google http client dependencies to v1.44.1 ([#2467](#2467)) ([87d1435](87d1435)) * update googleapis/java-cloud-bom digest to ac9893c ([#2472](#2472)) ([7fff34a](7fff34a)) * update grpc dependencies to v1.61.1 ([#2463](#2463)) ([9ec575b](9ec575b)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

product-auto-label bot added the size: s Pull request size is small. label Jan 12, 2024

lqiu96 mentioned this pull request Jan 12, 2024

google-cloud-compute: ImagesClient does not terminate, and hangs instead googleapis/google-cloud-java#10220

Closed

product-auto-label bot added size: xl Pull request size is extra large. and removed size: s Pull request size is small. labels Jan 17, 2024

fix: Cancel the Timeout Task for HttpJson

7f04a7f

lqiu96 force-pushed the cancel-timeout-task branch from 7c99d52 to 7f04a7f Compare January 17, 2024 16:08

product-auto-label bot added size: m Pull request size is medium. and removed size: xl Pull request size is extra large. labels Jan 17, 2024

chore: Use HttpJsonClientCallImpl in main branch

e6f7219

chore: Use gRPC client for gRPC test

8c466ec

lqiu96 added 4 commits January 17, 2024 13:29

Revert "chore: Use HttpJsonClientCallImpl in main branch"

e226b4a

This reverts commit e6f7219.

chore: Clean up ITClientShutdown

90c9c53

Merge remote-tracking branch 'origin/main' into cancel-timeout-task

1899391

chore: Add docs for terminating the client

da2d1db

lqiu96 added the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 29, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 29, 2024

lqiu96 marked this pull request as ready for review January 29, 2024 22:01

lqiu96 requested a review from a team as a code owner January 29, 2024 22:01

lqiu96 requested review from suztomo and blakeli0 January 29, 2024 22:01

chore: Remove duplicate isTerminated check

108dd47

lqiu96 requested review from diegomarquezp and JoeWang1127 February 5, 2024 21:26

JoeWang1127 reviewed Feb 6, 2024

View reviewed changes

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITClientShutdown.java Outdated Show resolved Hide resolved

chore: Fix sonar bugs

34f6e03

lqiu96 mentioned this pull request Feb 7, 2024

chore: Update showcase protos #2445

Merged

lqiu96 added 2 commits February 7, 2024 13:08

chore: Fix lint issues

50fab95

Merge branch 'main' into cancel-timeout-task

37fab75

product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Feb 7, 2024

lqiu96 added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 7, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 7, 2024

suztomo reviewed Feb 8, 2024

View reviewed changes

gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCallImpl.java Outdated Show resolved Hide resolved

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITClientShutdown.java Outdated Show resolved Hide resolved

lqiu96 added 4 commits February 8, 2024 16:36

chore: Add comment about future being null

28c1861

chore: Increase test timeout

83f9f80

Merge branch 'main' into cancel-timeout-task

fbe59f0

chore: Fix typo

25c5d0d

lqiu96 requested a review from suztomo February 9, 2024 17:13

suztomo approved these changes Feb 12, 2024

View reviewed changes

burkedavison approved these changes Feb 13, 2024

View reviewed changes

lqiu96 added 3 commits February 13, 2024 14:20

Merge branch 'main' into cancel-timeout-task

4e58707

chore: Refactor showcase tests

746c0c2

Merge branch 'main' into cancel-timeout-task

7aa1f35

lqiu96 merged commit b177d9e into main Feb 13, 2024
37 of 39 checks passed

lqiu96 deleted the cancel-timeout-task branch February 13, 2024 20:51

release-please bot mentioned this pull request Feb 13, 2024

chore(main): release 2.35.0 #2448

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Cancel the Timeout Task for HttpJson #2360

fix: Cancel the Timeout Task for HttpJson #2360

lqiu96 commented Jan 12, 2024 •

edited

Loading

suztomo commented Jan 12, 2024 •

edited

Loading

lqiu96 commented Jan 16, 2024

lqiu96 commented Jan 17, 2024

lqiu96 commented Jan 17, 2024

lqiu96 commented Jan 17, 2024

lqiu96 commented Jan 29, 2024 •

edited

Loading

suztomo commented Feb 8, 2024

lqiu96 commented Feb 9, 2024

suztomo left a comment

sonarcloud bot commented Feb 13, 2024

sonarcloud bot commented Feb 13, 2024

fix: Cancel the Timeout Task for HttpJson #2360

fix: Cancel the Timeout Task for HttpJson #2360

Conversation

lqiu96 commented Jan 12, 2024 • edited Loading

Local Testing

suztomo commented Jan 12, 2024 • edited Loading

lqiu96 commented Jan 16, 2024

lqiu96 commented Jan 17, 2024

lqiu96 commented Jan 17, 2024

lqiu96 commented Jan 17, 2024

lqiu96 commented Jan 29, 2024 • edited Loading

suztomo commented Feb 8, 2024

lqiu96 commented Feb 9, 2024

suztomo left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Feb 13, 2024

Quality Gate failed for 'gapic-generator-java-root'

sonarcloud bot commented Feb 13, 2024

Quality Gate failed for 'java_showcase_integration_tests'

lqiu96 commented Jan 12, 2024 •

edited

Loading

suztomo commented Jan 12, 2024 •

edited

Loading

lqiu96 commented Jan 29, 2024 •

edited

Loading