HTTPCLIENT-2152 #304
HTTPCLIENT-2152 #304
Conversation
|
@ok2c There is no |
|
@michael-o |
michael-o
left a comment
michael-o
left a comment
There was a problem hiding this comment.
I am not really qualified to made a judgement here. My async foo isn't existing. I only have a few inline comments, but will generally leave this to your judgement and other fellow committers.
| final HttpResponse response = new BasicHttpResponse(HttpStatus.SC_SERVICE_UNAVAILABLE); | ||
| response.addHeader(HttpHeaders.RETRY_AFTER, Long.toString(retryAfter.toSeconds())); | ||
| final ProtocolVersion version = request.getVersion(); | ||
| if (version != null && version.compareToVersion(HttpVersion.HTTP_2) < 0) { |
There was a problem hiding this comment.
Why close if version is 1.1+?
There was a problem hiding this comment.
@michael-o It is the other way around. Close connection If the connection is HTTP/1.1 or lower. HTTP/2 connections do not support Connection directives.
There was a problem hiding this comment.
Sorry, my question was misleading. Once again: Why close the connection of th version is exactly 1.1?
There was a problem hiding this comment.
@michael-o What is the point of keeping the connection alive if the server is unable to serve requests over it?
There was a problem hiding this comment.
Well, from an abstract point of view this connection is used to a host:port pair, but the URI path can be proxied to to other backend servers as well. E.g., example.com/service1 and example.com/service2 can be to completely different backends where service1 is offline, but service2 not.
There was a problem hiding this comment.
@michael-o I am not sure I understand what it is exactly you are proposing or objecting to.
There was a problem hiding this comment.
I am neither, I am just trying to clarify that the status code from the server applies solely to the path of the request and it does not give you any idicator that anything else served by this server is unavailable thus making the entire connection invalid . I don't consider the connection to be invalid unless TCP tells me otherwise or the server tells me to close.
There was a problem hiding this comment.
@michael-o This is a test class. It is there to merely validate the assumptions of the test case.
There was a problem hiding this comment.
Duh, I am an idiot. I didn't notice this. So this spot is fine. Sorry for the noise.
There was a problem hiding this comment.
@michael-o Not at all. Thank you for a thorough review!
...nt5/src/main/java/org/apache/hc/client5/http/impl/async/InternalAbstractHttpAsyncClient.java
Outdated
Show resolved
Hide resolved
...nt5/src/main/java/org/apache/hc/client5/http/impl/async/InternalAbstractHttpAsyncClient.java
Outdated
Show resolved
Hide resolved
…the async request retry exec interceptor
|
For my uneducated eye this looks fine now. |
|
We need to make the I/O reactor more robust. The current amount of surface area for catastrophic production bugs in the async client is totally unacceptable. |
3 participants
@rschmitt Please let me know if you have any objections to this change-set.
@michael-o Please also take a look at
HttpRequestRetryStrategyrelated changes and let me know if everything looks OK.