Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix watchdog NPE red herring #1344

Merged
merged 4 commits into from Apr 14, 2021
Merged

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 as code owners Apr 8, 2021
@google-cla google-cla bot added the cla: yes label Apr 8, 2021
@codecov
Copy link

@codecov 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

anyway to test this?

elharo
elharo approved these changes Apr 8, 2021
callable1.call("request", observer);
// This should cancel callable1
watchdog.run();
assertThat(callable1.popLastCall().getController().isCancelled()).isTrue();
Copy link
Contributor

@elharo elharo Apr 8, 2021

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 label Apr 13, 2021
@mutianf
Copy link
Contributor Author

@mutianf mutianf commented Apr 13, 2021

@igorbernstein2 igorbernstein2 merged commit 06dbf12 into googleapis:master Apr 14, 2021
15 checks passed
@mutianf mutianf deleted the watchdog_npe branch May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes kokoro:force-run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants