-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[UNDERTOW-2249] Change that HttpClientConnection.sendRequest on a clo... #1564
Conversation
Hi @fl4via , could you please review my PR? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xjusko the ClosedChannelException is not always appropriate, as if you look at the usages of UndertowClientMessages.invalidConnectionState, it is thrown not only when the connection is closed or close is requested, but also on upgrade scenarios.
That being said, I see the point you raised and I think the ticket is valid. So, my suggestion here is to add another message just for closed connections, that message would throw the ClosedChannelException. Then, at the usage points, you would have two checks: one for upgrades, that uses the ld IOException invalidConnectionState, and the other one for close states, and this one would use your new message and throw the ClosedChannelException.
…sed connection results in a ClosedChannelException instead of IOException
Thanks for the review @fl4via , it should be fixed based on your suggestion. |
@fl4via I see that CI failed, but I don't think that my changes have any implications on the test that failed. |
@xjusko ^^ |
...sed connection results in a ClosedChannelException instead of IOException
https://issues.redhat.com/browse/UNDERTOW-2249