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

Do not warn blocking future operations when done #2564

Merged
merged 1 commit into from Mar 10, 2020

Conversation

@trustin
Copy link
Member

trustin commented Mar 9, 2020

Motivation:

Blocking future operations never block when the future is done, and thus
we should not warn about such cases.

Modifications:

  • Do not warn when the future being checked is done.
  • Update and clean up the test case
  • Make CompletableRpcResponse and AbstractRequestContextAwareFuture
    extend EventLoopCheckingFuture to reduce code duplication.
  • Move EventLoopCheckingUtil back into EventLoopCheckingFuture.
  • Miscellaneous:
    • Keep the reported threads in a concurrent map.

Result:

  • No false positive warnings
  • Less code duplication
  • Less contention
Motivation:

Blocking future operations never block when the future is done, and thus
we should not warn about such cases.

Modifications:

- Do not warn when the future being checked is done.
- Update and clean up the test case
- Make `CompletableRpcResponse` and `AbstractRequestContextAwareFuture`
  extend `EventLoopCheckingFuture` to reduce code duplication.
- Move `EventLoopCheckingUtil` back into `EventLoopCheckingFuture`.
- Miscellaneous:
  - Keep the reported threads in a concurrent map.

Result:

- No false positive warnings
- Less code duplication
- Less contention
@trustin trustin added the defect label Mar 9, 2020
@trustin trustin added this to the 0.99.0 milestone Mar 9, 2020
@trustin trustin requested review from ikhoon and minwoox as code owners Mar 9, 2020
@trustin

This comment has been minimized.

Copy link
Member Author

trustin commented Mar 9, 2020

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 9, 2020

Codecov Report

Merging #2564 into master will increase coverage by 0.01%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2564      +/-   ##
============================================
+ Coverage     73.55%   73.56%   +0.01%     
+ Complexity    10930    10927       -3     
============================================
  Files           957      956       -1     
  Lines         41860    41846      -14     
  Branches       5238     5238              
============================================
- Hits          30789    30786       -3     
+ Misses         8375     8365      -10     
+ Partials       2696     2695       -1
Impacted Files Coverage Δ Complexity Δ
...rnal/common/AbstractRequestContextAwareFuture.java 81.81% <100%> (+8.64%) 11 <0> (-2) ⬇️
...inecorp/armeria/common/CompletableRpcResponse.java 48% <100%> (-3.62%) 8 <1> (-2)
...p/armeria/common/util/EventLoopCheckingFuture.java 91.3% <77.77%> (-8.7%) 10 <4> (+4)
.../reactive/ArmeriaHttpClientResponseSubscriber.java 66.66% <0%> (-2.39%) 11% <0%> (ø)
...om/linecorp/armeria/client/HttpSessionHandler.java 59.84% <0%> (-2.37%) 28% <0%> (-2%)
.../com/linecorp/armeria/server/RoutingPredicate.java 69.35% <0%> (-1.62%) 20% <0%> (-1%)
...java/com/linecorp/armeria/server/DefaultRoute.java 95.42% <0%> (-0.66%) 73% <0%> (-1%)
...com/linecorp/armeria/server/saml/SamlEndpoint.java 65% <0%> (+2.5%) 11% <0%> (+1%) ⬆️
...rp/armeria/common/stream/DefaultStreamMessage.java 89.71% <0%> (+3.42%) 65% <0%> (+2%) ⬆️
...meria/common/stream/RegularFixedStreamMessage.java 87.75% <0%> (+6.12%) 14% <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 98c2e7a...de05be2. Read the comment docs.

@ikhoon
ikhoon approved these changes Mar 9, 2020
Copy link
Contributor

ikhoon left a comment

Thanks!

@renaudb
renaudb approved these changes Mar 9, 2020
Copy link
Member

minwoox left a comment

👍

@trustin trustin merged commit 3627b97 into line:master Mar 10, 2020
4 checks passed
4 checks passed
codecov/patch 81.81% of diff hit (target 73.55%)
Details
codecov/project 73.56% (+0.01%) compared to 98c2e7a
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@trustin trustin deleted the trustin:do_not_warn_blocking_operation_if_done branch Mar 10, 2020
@trustin

This comment has been minimized.

Copy link
Member Author

trustin commented Mar 10, 2020

Thanks for reviewing!

trustin added a commit that referenced this pull request Mar 13, 2020
Motivation:

Blocking future operations never block when the future is done, and thus
we should not warn about such cases.

Modifications:

- Do not warn when the future being checked is done.
- Update and clean up the test case
- Make `DefaultRpcResponse` and `AbstractRequestContextAwareFuture`
  extend `EventLoopCheckingFuture` to reduce code duplication.
- Move `EventLoopCheckingUtil` back into `EventLoopCheckingFuture`.
- Miscellaneous:
  - Keep the reported threads in a concurrent map.

Result:

- No false positive warnings
- Less code duplication
- Less contention
@trustin trustin modified the milestones: 0.99.0, 0.98.5 Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.