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

fix: fix watchdog NPE red herring #1344

Merged
merged 4 commits into from
Apr 14, 2021

Conversation

mutianf
Copy link
Contributor

@mutianf mutianf commented Apr 8, 2021

If CPU is overloaded, it's possible that the stream doesn't get started after idle timeout. In this case, innerController is not set and innerController.cancel() will throw NPE. The NPE is caught in run() and watchdog will still behave correctly, but the exception could be confusing. Adding a check to skip cancelIfStale to avoid the red herring.

@mutianf mutianf requested review from a team as code owners April 8, 2021 21:29
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 8, 2021
@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #1344 (72b8dc3) into master (32240eb) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1344   +/-   ##
=========================================
  Coverage     81.42%   81.43%           
  Complexity     1346     1346           
=========================================
  Files           211      211           
  Lines          5728     5730    +2     
  Branches        526      527    +1     
=========================================
+ Hits           4664     4666    +2     
  Misses          851      851           
  Partials        213      213           
Impacted Files Coverage Δ Complexity Δ
...src/main/java/com/google/api/gax/rpc/Watchdog.java 79.83% <100.00%> (+0.33%) 12.00 <0.00> (ø)
.../java/com/google/api/gax/batching/BatcherImpl.java 94.94% <0.00%> (-1.13%) 22.00% <0.00%> (-1.00%)
.../google/api/gax/batching/NonBlockingSemaphore.java 81.57% <0.00%> (+5.26%) 12.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32240eb...72b8dc3. Read the comment docs.

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anyway to test this?

callable1.call("request", observer);
// This should cancel callable1
watchdog.run();
assertThat(callable1.popLastCall().getController().isCancelled()).isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use of truth here feels gratuitous since assertTrue is simple and obvious

@igorbernstein2 igorbernstein2 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 13, 2021
@mutianf
Copy link
Contributor Author

mutianf commented Apr 13, 2021

@googlebot

@igorbernstein2 igorbernstein2 merged commit 06dbf12 into googleapis:master Apr 14, 2021
@mutianf mutianf deleted the watchdog_npe branch May 13, 2021 17:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement. kokoro:force-run Add this label to force Kokoro to re-run the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants