-
Notifications
You must be signed in to change notification settings - Fork 16
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
Dialogue guards against HTTPCLIENT-1924 client self-closure #1951
Conversation
Generate changelog in
|
86f3e7b
to
7111a5f
Compare
|
||
@Override | ||
public void writeTo(OutputStream _output) { | ||
throw new StackOverflowError(); |
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.
This makes for a nice test, and I suspect is similar to the issue which prompted this change, but in practice I don't think httpclient should actually close the connection pool when an error is thrown from this portion of the request. I will follow up separately on that.
* https://github.com/apache/httpcomponents-client/blob/5b61e132c3871ddfa967ab21b3af5d6d738bc6e8/ | ||
* httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/MainClientExec.java#L161-L164 |
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.
hyperlink: MainClientExec.java#L161-L164
Released 3.85.0 |
Before this PR
The underlying http client closes itself when an Error is thrown.
After this PR
==COMMIT_MSG==
Dialogue guards against HTTPCLIENT-1924 client self-closure
==COMMIT_MSG==
Possible downsides?
In cases that would previously self-close, we're guaranteed that a connection will leak. Our connection pool allows two billion entries, so it's not the worst thing in the world so long as it doesn't happen in a hot loop, which is unlikely, and in the unlikely worst case causes similar issues to the self-closure problem.