Aias00 commented on code in PR #7265:
URL: https://github.com/apache/shenyu/pull/7265#discussion_r4110185383
##########
shenyu-spring-boot-starter/shenyu-spring-boot-starter-plugin/shenyu-spring-boot-starter-plugin-httpclient/src/main/java/org/apache/shenyu/springboot/starter/plugin/httpclient/HttpClientFactory.java:
##########
@@ -113,6 +113,10 @@ protected HttpClient createInstance() {
ConnectionProvider connectionProvider = buildConnectionProvider(pool);
HttpClient httpClient = HttpClient.create(connectionProvider)
.option(ChannelOption.CONNECT_TIMEOUT_MILLIS,
properties.getConnectTimeout());
+ Duration responseTimeout = properties.getResponseTimeout();
+ if (!responseTimeout.isZero() && !responseTimeout.isNegative()) {
+ httpClient = httpClient.responseTimeout(responseTimeout);
Review Comment:
Important non-blocking finding, from running this against reactor-netty
1.1.19 (the version resolved by Spring Boot 3.3.1 here).
`responseTimeout` installs Netty `ReadTimeoutHandler` after the request is
sent and removes it only when the exchange terminates
(HttpClientOperations.java:645-658) - so it caps the gap between consecutive
reads **for the whole response including the body**, not time-to-first-byte.
Loopback server flushes headers immediately, then emits chunks:
- responseTimeout 500ms, gap 1.5s -> `ReadTimeoutException` after 0 chunks
(591 ms)
- responseTimeout 500ms, gap 100ms -> completed, 5 chunks
- responseTimeout 3000ms (the default), gap 5s -> `ReadTimeoutException`
after 0 chunks (3081 ms)
- responseTimeout 3000ms (the default), gap 1.5s -> completed, 3 chunks
Second part, which is why I flagged this rather than just noting it: the
factory **already** installs `ReadTimeoutHandler(readTimeout)` further down in
`doOnConnected` (HttpClientFactory.java:126-131) and `readTimeout` also
defaults to 3000. With `responseTimeout` unset that handler still aborts the
5s-gap stream at 3078 ms, and setting `responseTimeout: 0` does not help either
- aborted at 3003 ms. Only zeroing both keeps a slow stream open.
So SSE / long polling / AI token streaming need `responseTimeout: 0` **and**
`readTimeout: 0`. Please record that interaction in the javadoc of
`HttpClientProperties#responseTimeout` and `#readTimeout`, or add a test that
pins it - as of today a "responseTimeout = 0 keeps the stream alive" test would
fail because of `readTimeout`, so we should answer it deliberately rather than
discover it later.
--
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]
