Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
ok2c commented on PR #694: URL: https://github.com/apache/httpcomponents-client/pull/694#issuecomment-3166853308 > Is there any plan to support 10ms granularity timeout control in async connections in the fulture? @lethinker No, there is not. Please note that the solution mentioned by @rschmitt will cause the JRE run with near 100% CPU utilization. This is not a problem with a short integration test but may be a problem when running in PROD -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
lethinker commented on PR #694: URL: https://github.com/apache/httpcomponents-client/pull/694#issuecomment-3166391150 > @lethinker https://github.com/apache/httpcomponents-client/blob/master/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/extension/async/StandardTestClientBuilder.java#L191 Thank you -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
rschmitt commented on PR #694: URL: https://github.com/apache/httpcomponents-client/pull/694#issuecomment-3166384261 @lethinker https://github.com/apache/httpcomponents-client/blob/master/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/extension/async/StandardTestClientBuilder.java#L191 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
lethinker commented on PR #694: URL: https://github.com/apache/httpcomponents-client/pull/694#issuecomment-3166241851 > > It seems that the effected ResponseTimeout is being rounded up to the nearest second, and milliseconds are not taking effect. > > @lethinker Socket timeout granularity of async connections is one second (can be reduced at the cost of the i/o reactor running in a tighter loop and waking up more often, and causing higher CPU utilization). Socket timeout granularity of blocking connections is _approximately_ 10 ms. > > Timeouts in ms make no sense. Thank you for your help. Is there any plan to support 10ms granularity timeout control in async connections in the fulture? I just have the business scenario to use a timeout interval of 100ms, If the response time exceeds 100ms, I want to discard the message. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
ok2c commented on PR #694: URL: https://github.com/apache/httpcomponents-client/pull/694#issuecomment-3164323292 > It seems that the effected ResponseTimeout is being rounded up to the nearest second, and milliseconds are not taking effect. @lethinker Socket timeout granularity of async connections is one second (can be reduced at the cost of the i/o reactor running in a tighter loop and waking up more often, and causing higher CPU utilization). Socket timeout granularity of blocking connections is _approximately_ 10 ms. Timeouts in ms make no sense. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
lethinker commented on PR #694:
URL:
https://github.com/apache/httpcomponents-client/pull/694#issuecomment-3164276512
> @ok2c @rschmitt can you help me with the problem when I use the
httpAsyncClient( httpclient 5.4.3 、jdk 21) First, I set a 100-second sleep on
the server side to ensure that no response is returned within 100 seconds.
Second I use the `RequestConfig.setResponseTimeout(Timeout.ofMilliseconds(50))`
to set the response timeout value on the client side.
>
> I have modified the timeout multiple times and obtained the following
results. `[async]failed is 1 MILLISECONDS, time usage is 1033`（1 MILLISECONDS
means I set the ResponseTimeout as 1ms, time usage means that client side
timeout after 1033ms） `[async]failed is 10 MILLISECONDS, time usage is 1010`
`[async]failed is 500 MILLISECONDS, time usage is 1015`
>
> `[async]failed is 1000 MILLISECONDS, time usage is 1013` This is the
expected value. `[async]failed is 1020 MILLISECONDS, time usage is 2019` ..
>
> **It seems that the effected ResponseTimeout is being rounded up to the
nearest second, and milliseconds are not taking effect.**
>
> In addition, I used a synchronous httpclient, which works well. I set the
ResponseTimeout = 50ms. `[sync]failed is Read timed out, time usage is 63`
my code is
`
@Service
@EnableScheduling
public class TestService implements InitializingBean {
private CloseableHttpAsyncClient httpAsyncClient;
private HttpClient httpClient;
// SET the ResponseTimeout
private RequestConfig requestConfig = RequestConfig.custom()
.setConnectionRequestTimeout(Timeout.ofMilliseconds(500))
.setResponseTimeout(50, TimeUnit.MILLISECONDS)
.build();
@Scheduled(initialDelay = 0L, fixedRateString = "5000")
public void executeTask()
throws NoSuchFieldException, IllegalAccessException,
ExecutionException, InterruptedException {
testSyncDefaultTime();
testAsyncDefaultTime();
}
private void testAsyncDefaultTime()
throws NoSuchFieldException, IllegalAccessException,
ExecutionException, InterruptedException {
// create post request
SimpleHttpRequest request = new SimpleHttpRequest("POST",
"http://127.0.0.1:/timeout90s";);
long start = System.currentTimeMillis();
Future responseFuture =
httpAsyncClient.execute(request, new FutureCallback<>() {
@Override
public void completed(SimpleHttpResponse simpleHttpResponse) {
System.out.println(
"[async]response is " + simpleHttpResponse + ", time
usage is " + (System.currentTimeMillis() - start));
}
@Override
public void failed(Exception e) {
System.out.println(
"[async]failed is " + e.getMessage() + ", time usage is
" + (System.currentTimeMillis() - start));
}
@Override
public void cancelled() {
System.out.println("[async]request is cancelled, time usage
is " + (System.currentTimeMillis() - start));
}
});
responseFuture.get();
}
private void testSyncDefaultTime() {
BasicClassicHttpRequest httpRequest = new
BasicClassicHttpRequest("POST",
"http://127.0.0.1:/huaweidns/timeout90s";);
long start = System.currentTimeMillis();
HttpResponse response;
try {
response = httpClient.execute(httpRequest);
} catch (Exception e) {
System.out.println(
"[sync]failed is " + e.getMessage() + ", time usage is " +
(System.currentTimeMillis() - start));
return;
}
System.out.println("[sync]response is " + response + ", time usage
is " + (System.currentTimeMillis() - start));
}
@Override
public void afterPropertiesSet() {
IOReactorConfig ioReactorConfig = IOReactorConfig.custom()
.setSoTimeout(50, TimeUnit.MILLISECONDS) // 设置I/O反应器的超时时间为50毫秒
.setIoThreadCount(8)
.build();
httpAsyncClient = HttpAsyncClientBuilder.create()
.setDefaultRequestConfig(requestConfig)
.setIOReactorConfig(ioReactorConfig)
.build();
httpAsyncClient.start();
httpClient =
HttpClients.custom().setDefaultRequestConfig(requestConfig).build();
}
}`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
us...@infra.
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
lethinker commented on PR #694: URL: https://github.com/apache/httpcomponents-client/pull/694#issuecomment-3164182716 @ok2c @rschmitt can you help me with the problem when I use the httpAsyncClient( httpclient 5.4.3 、jdk 21) First, I set a 100-second sleep on the server side to ensure that no response is returned within 100 seconds. Second I use the `RequestConfig.setResponseTimeout(Timeout.ofMilliseconds(50))` to set the response timeout value on the client side. I have modified the timeout multiple times and obtained the following results. `[async]failed is 1 MILLISECONDS, time usage is 1033` `[async]failed is 10 MILLISECONDS, time usage is 1010` `[async]failed is 500 MILLISECONDS, time usage is 1015` `[async]failed is 1000 MILLISECONDS, time usage is 1013` This is the expected value. `[async]failed is 1020 MILLISECONDS, time usage is 2019` .. **It seems that the effected ResponseTimeout is being rounded up to the nearest second, and milliseconds are not taking effect.** In addition, I used a synchronous httpclient, which works well. I set the ResponseTimeout = 50ms. `[sync]failed is Read timed out, time usage is 63` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
ok2c commented on code in PR #694:
URL:
https://github.com/apache/httpcomponents-client/pull/694#discussion_r2259331530
##
httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/DefaultAsyncClientConnectionOperator.java:
##
@@ -165,7 +165,13 @@ public void completed(final IOSession session) {
if (tlsStrategy != null) {
try {
final Timeout socketTimeout =
connection.getSocketTimeout();
-final Timeout handshakeTimeout =
tlsConfig.getHandshakeTimeout();
+// TLS handshake timeout precedence:
+// 1. Explicitly configured handshake timeout
from TlsConfig
+// 2. Current socket timeout of the connection
(if set)
+// 3. Falls back to connectTimeout if neither
is specified (handled later)
+final Timeout handshakeTimeout =
tlsConfig.getHandshakeTimeout() != null
Review Comment:
@rschmitt Ubuntu (or Debian) Linux is my primary development environment but
sometimes I have to use a client issued laptop that runs Windows. I run Maven
inside WinGit console. It is an old habit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
rschmitt commented on code in PR #694:
URL:
https://github.com/apache/httpcomponents-client/pull/694#discussion_r2258400165
##
httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/DefaultAsyncClientConnectionOperator.java:
##
@@ -165,7 +165,13 @@ public void completed(final IOSession session) {
if (tlsStrategy != null) {
try {
final Timeout socketTimeout =
connection.getSocketTimeout();
-final Timeout handshakeTimeout =
tlsConfig.getHandshakeTimeout();
+// TLS handshake timeout precedence:
+// 1. Explicitly configured handshake timeout
from TlsConfig
+// 2. Current socket timeout of the connection
(if set)
+// 3. Falls back to connectTimeout if neither
is specified (handled later)
+final Timeout handshakeTimeout =
tlsConfig.getHandshakeTimeout() != null
Review Comment:
You're doing Windows development in mingw? Not WSL2 or native Windows?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
arturobernalg closed pull request #694: HTTPCLIENT-2386: Fix TLS handshake timeout precedence URL: https://github.com/apache/httpcomponents-client/pull/694 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
arturobernalg commented on PR #694: URL: https://github.com/apache/httpcomponents-client/pull/694#issuecomment-3155072359 Closing in favor of [88c19c0](https://github.com/apache/httpcomponents-client/pull/699/commits/88c19c009628750fa6f84b7635fc46b72e439edb) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
ok2c commented on code in PR #694:
URL:
https://github.com/apache/httpcomponents-client/pull/694#discussion_r2253819023
##
httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/DefaultAsyncClientConnectionOperator.java:
##
@@ -165,7 +165,13 @@ public void completed(final IOSession session) {
if (tlsStrategy != null) {
try {
final Timeout socketTimeout =
connection.getSocketTimeout();
-final Timeout handshakeTimeout =
tlsConfig.getHandshakeTimeout();
+// TLS handshake timeout precedence:
+// 1. Explicitly configured handshake timeout
from TlsConfig
+// 2. Current socket timeout of the connection
(if set)
+// 3. Falls back to connectTimeout if neither
is specified (handled later)
+final Timeout handshakeTimeout =
tlsConfig.getHandshakeTimeout() != null
Review Comment:
@rschmitt Fair enough. Please take a look at an alternative #699 and
corrects the behavior of the classic connection operator instead.
With regards to the connect timeout integration tests they consistently fail
for me locally (Windows MINGW64_NT-10.0-26100). I need to get them fixed first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
rschmitt commented on code in PR #694:
URL:
https://github.com/apache/httpcomponents-client/pull/694#discussion_r2252475258
##
httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/DefaultAsyncClientConnectionOperator.java:
##
@@ -165,7 +165,13 @@ public void completed(final IOSession session) {
if (tlsStrategy != null) {
try {
final Timeout socketTimeout =
connection.getSocketTimeout();
-final Timeout handshakeTimeout =
tlsConfig.getHandshakeTimeout();
+// TLS handshake timeout precedence:
+// 1. Explicitly configured handshake timeout
from TlsConfig
+// 2. Current socket timeout of the connection
(if set)
+// 3. Falls back to connectTimeout if neither
is specified (handled later)
+final Timeout handshakeTimeout =
tlsConfig.getHandshakeTimeout() != null
Review Comment:
It should default to the `connectTimeout`. The whole point of having
separate TLS handshake timeout configuration is that defaulting to the
`socketTimeout` (which is what would naturally happen) means that TLS
handshakes take at least an order of magnitude longer to time out than is
sensible. Response data can take an arbitrarily long amount of time to come
back, whereas a TLS handshake should take roughly `2*RTT` irrespective of the
nature of the request being sent. If there's an inconsistency here between
classic and async then it sounds like the classic behavior is wrong.
It also sounds like we could use an integration test here [similar to the
one for socket
timeouts](https://github.com/apache/httpcomponents-client/blob/master/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestSocketTimeout.java#L63-L77),
which can also be set in a variety of ways. Such coverage could be added to
[the existing integration tests for socket
timeouts](https://github.com/apache/httpcomponents-client/blob/master/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestSocketTimeout.java#L63-L77).
I added these tests in order to prevent _precisely_ these kinds of regressions
which I've had to deal with in the past, and it's not good that a change like
the one in this PR doesn't break any test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
ok2c commented on code in PR #694:
URL:
https://github.com/apache/httpcomponents-client/pull/694#discussion_r2246069191
##
httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/DefaultAsyncClientConnectionOperator.java:
##
@@ -165,7 +165,13 @@ public void completed(final IOSession session) {
if (tlsStrategy != null) {
try {
final Timeout socketTimeout =
connection.getSocketTimeout();
-final Timeout handshakeTimeout =
tlsConfig.getHandshakeTimeout();
+// TLS handshake timeout precedence:
+// 1. Explicitly configured handshake timeout
from TlsConfig
+// 2. Current socket timeout of the connection
(if set)
+// 3. Falls back to connectTimeout if neither
is specified (handled later)
+final Timeout handshakeTimeout =
tlsConfig.getHandshakeTimeout() != null
Review Comment:
@rschmitt Would this change be OK with you? This should fix an inconsistency
in the behavior of the classic and async transports.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
ok2c commented on code in PR #694:
URL:
https://github.com/apache/httpcomponents-client/pull/694#discussion_r2246066138
##
httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/DefaultAsyncClientConnectionOperator.java:
##
@@ -165,7 +165,13 @@ public void completed(final IOSession session) {
if (tlsStrategy != null) {
try {
final Timeout socketTimeout =
connection.getSocketTimeout();
-final Timeout handshakeTimeout =
tlsConfig.getHandshakeTimeout();
+// TLS handshake timeout precedence:
+// 1. Explicitly configured handshake timeout
from TlsConfig
+// 2. Current socket timeout of the connection
(if set)
+// 3. Falls back to connectTimeout if neither
is specified (handled later)
Review Comment:
@arturobernalg I am not sure this is correct. Connect timeout should no
longer have any effect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
[PR] HTTPCLIENT-2386: Fix TLS handshake timeout precedence [httpcomponents-client]
arturobernalg opened a new pull request, #694: URL: https://github.com/apache/httpcomponents-client/pull/694 Changes TLS handshake timeout fallback from `connectTimeout` to `socketTimeout` when no explicit timeout is configured in `TlsConfig`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
