Skip to content

Refactor HaProxy exception handling - #5805

Merged
ikhoon merged 1 commit into
line:mainfrom
jrhee17:refactor/haproxy-request
Jul 25, 2024
Merged

Refactor HaProxy exception handling#5805
ikhoon merged 1 commit into
line:mainfrom
jrhee17:refactor/haproxy-request

Conversation

@jrhee17

@jrhee17 jrhee17 commented Jul 9, 2024

Copy link
Copy Markdown
Contributor

Motivation:

While working on #5793, I found that there were unnecessary ProxyConnectException related handling in the request handler. This is most likely due to the fact that HaProxyHandler works differently from the other proxy handlers in that the session is completed, and then a connection event is published.

This was probably when I was less familiar with netty. I propose that the HaProxyHandler complete the session similarly to the other handlers. A connection attempt is made first, and then the sessionPromise is succeeded or failed accordingly.

Modifications:

  • HaProxyHandler doesn't succeed the sessionPromise immediately, but waits for the result of the connect future.
  • Remove ProxyConnectException related logic in all places except for HttpSessionHandler#exceptionCaught

Result:

  • The exception handling in AbstractHttpRequestHandler and HttpSessionHandler is a little simpler.

@jrhee17 jrhee17 added the cleanup label Jul 9, 2024
@jrhee17 jrhee17 added this to the 1.30.0 milestone Jul 9, 2024

@trustin trustin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The simplification looks fine to me. I don't remember anymore why it is written that way anymore to be honest 😅

@jrhee17
jrhee17 marked this pull request as ready for review July 9, 2024 23:57
@jrhee17
jrhee17 requested review from ikhoon and minwoox as code owners July 9, 2024 23:57

@minwoox minwoox left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! 👍 👍 👍

@ikhoon ikhoon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

@ikhoon
ikhoon merged commit 450d493 into line:main Jul 25, 2024
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.

4 participants