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

Fail immediately on proxy connect exception #2805

Merged
merged 3 commits into from Jun 18, 2020

Conversation

jrhee17
Copy link
Contributor

@jrhee17 jrhee17 commented Jun 17, 2020

Motivation
Related: #2801

Proxy client can hang when a connection is established, but the connection is closed without a ProxyConnectEvent.

  1. If a ProxyConnectEvent is never received, we don't mark the session as complete.
  2. Since the session isn't completed, the first write is never invoked
  3. Previously we had been relying on ProxyHandlers pending write failures to be notified of proxy failure (HttpRequestSubscriber.operationComplete used to close the response previously).

Modification
Try to fail the sessionPromise on proxy connect exception

@jrhee17 jrhee17 force-pushed the bugfix/investigate-proxy-hang branch from 83dfb04 to 1c0af1f Compare June 17, 2020 13:17
@jrhee17 jrhee17 changed the title investigate proxy client hang on immediate close Fail immediately on proxy exception Jun 17, 2020
@jrhee17 jrhee17 changed the title Fail immediately on proxy exception Fail immediately on proxy connect exception Jun 17, 2020
@ikhoon ikhoon added the defect label Jun 17, 2020
@ikhoon ikhoon added this to the 0.99.7 milestone Jun 17, 2020
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this. 😄

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @jrhee17 !

@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #2805 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2805      +/-   ##
============================================
- Coverage     72.80%   72.72%   -0.08%     
- Complexity    11929    11934       +5     
============================================
  Files          1049     1049              
  Lines         46467    46554      +87     
  Branches       5810     5810              
============================================
+ Hits          33828    33856      +28     
- Misses         9665     9724      +59     
  Partials       2974     2974              
Impacted Files Coverage Δ Complexity Δ
...om/linecorp/armeria/client/HttpSessionHandler.java 67.05% <ø> (-1.02%) 48.00 <0.00> (-1.00)
...inecorp/armeria/common/logging/RequestOnlyLog.java 20.00% <0.00%> (-46.67%) 2.00% <0.00%> (ø%)
...om/linecorp/armeria/common/logging/RequestLog.java 38.46% <0.00%> (-44.88%) 4.00% <0.00%> (ø%)
.../armeria/server/logging/LoggingServiceBuilder.java 72.09% <0.00%> (-19.09%) 18.00% <0.00%> (ø%)
...inecorp/armeria/common/ClosedSessionException.java 36.36% <0.00%> (-18.19%) 4.00% <0.00%> (-1.00%)
...p/armeria/client/logging/LoggingClientBuilder.java 50.00% <0.00%> (-14.52%) 9.00% <0.00%> (ø%)
...rmeria/common/logging/LoggingDecoratorBuilder.java 63.63% <0.00%> (-13.94%) 38.00% <0.00%> (ø%)
...rmeria/client/logging/LoggingRpcClientBuilder.java 12.50% <0.00%> (-3.63%) 5.00% <0.00%> (ø%)
...rp/armeria/internal/common/Http2GoAwayHandler.java 62.50% <0.00%> (-2.50%) 16.00% <0.00%> (-1.00%)
.../main/java/com/linecorp/armeria/server/Server.java 80.14% <0.00%> (-1.11%) 36.00% <0.00%> (ø%)
... and 31 more

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 f5e2a55...909d712. Read the comment docs.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks! @jrhee17 🙇‍♂️

@trustin trustin merged commit eabd42b into line:master Jun 18, 2020
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
Motivation:

Related: line#2801 

Proxy client can hang when a connection is established, but the connection is closed without a `ProxyConnectEvent`.
1) If a `ProxyConnectEvent` is never received, we don't mark the session as complete.
2) Since the session isn't completed, the first write is never invoked
3) Previously we had been relying on `ProxyHandler`s pending write failures to be notified of proxy failure (`HttpRequestSubscriber.operationComplete` used to close the response previously). 

Modification:

Try to fail the `sessionPromise` on proxy connect exception
@jrhee17 jrhee17 deleted the bugfix/investigate-proxy-hang branch August 30, 2021 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants