Aias00 commented on code in PR #7162:
URL: https://github.com/apache/shenyu/pull/7162#discussion_r4071696365
##########
shenyu-plugin/shenyu-plugin-httpclient/src/main/java/org/apache/shenyu/plugin/httpclient/config/HttpClientProperties.java:
##########
@@ -396,9 +396,9 @@ public void setMaxInMemorySize(final Integer
maxInMemorySize) {
public static class Pool {
/**
- * Type of pool for HttpClient to use, defaults to ELASTIC.
+ * Type of pool for HttpClient to use, defaults to FIXED.
*/
- private PoolType type = PoolType.ELASTIC;
+ private PoolType type = PoolType.FIXED;
Review Comment:
Switching the default here changes more than the pool type — it also
switches on an unbounded pending queue with a 45s wait.
`HttpClientFactory#buildConnectionProvider` routes to two very different
configurations:
```java
// ELASTIC (old default)
builder.maxConnections(Integer.MAX_VALUE)
.pendingAcquireTimeout(Duration.ofMillis(0));
// FIXED (new default)
builder.maxConnections(pool.getMaxConnections()) // default 500
.pendingAcquireTimeout(Duration.ofMillis(pool.getAcquireTimeout()))
// default 45000
.pendingAcquireMaxCount(-1); // unlimited
pending
```
So a deployment that upgrades without touching any config goes from "open
another connection" to "queue without limit and hang for up to 45 seconds" once
500 concurrent connections are in use — and `pendingAcquireMaxCount(-1)` means
the queue itself is unbounded, so this fails by accumulating latency and memory
rather than by rejecting.
Bounding the pool is the right instinct (an ELASTIC pool is an unbounded
resource), but the two defaults have to be chosen together. Concretely:
- lower the default `acquireTimeout` for a gateway — 45s exceeds almost
every client and upstream timeout, so a saturated pool turns into requests that
hang long after the caller gave up; something in the 1-5s range fails fast and
lets the load balancer/retry logic react;
- and/or give `pendingAcquireMaxCount` a real bound instead of `-1`, so
saturation produces a rejection instead of an ever-growing queue.
Either is fine, but please make it an explicit decision rather than
inheriting Reactor Netty's 45s default. This also deserves a release note: it
is a behavioural default change that affects every deployment that does not
configure `shenyu.httpclient.pool.*`.
--
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]
