envoyproxy / java-control-plane

Java implementation of an Envoy gRPC control plane
Apache License 2.0
293 stars 136 forks source link

Handle call cancellation gracefully #80

Closed sschepens closed 5 years ago

sschepens commented 5 years ago

Fixes #79

Immediately cancel watches when responseObserver is cancelled. Catch cancellation exceptions when responding watches. Correctly handle cancellations on error.

sschepens commented 5 years ago

@snowp @joeyb i'm inclined to remove WatchCancelledException from Watch#respond it seems as though we sould handle cancellations gracefully as they occur in normal situations, and handling them through exceptions doesn't seem right, too much overhead.

What do you think?

sschepens commented 5 years ago

I'm also not entirely sure how to test this changes, I don't know how to trigger a cancellation through tests

snowp commented 5 years ago

To test I think you want to call onError(Status.CANCELED) on the client stream observer. From what I can tell this will send an RST_STREAM to the server which it will treat as a canceled stream

codecov-io commented 5 years ago

Codecov Report

Merging #80 into master will decrease coverage by 0.42%. The diff coverage is 94.87%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #80      +/-   ##
============================================
- Coverage     94.82%   94.39%   -0.43%     
- Complexity      130      131       +1     
============================================
  Files            13       13              
  Lines           502      517      +15     
  Branches         44       46       +2     
============================================
+ Hits            476      488      +12     
- Misses           18       21       +3     
  Partials          8        8
Impacted Files Coverage Δ Complexity Δ
...nvoyproxy/controlplane/server/DiscoveryServer.java 96.29% <94.87%> (-2.63%) 13 <0> (+1)

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 aa7cc51...ae860cd. Read the comment docs.

sschepens commented 5 years ago

@snowp added a test for this, please review