Aias00 commented on PR #6414: URL: https://github.com/apache/shenyu/pull/6414#issuecomment-5156776421
Reviewing #6414. The body-replay fix is the right idea and the test coverage for the POST retry path is solid, but there's a regression I think needs to be addressed before merge. **Blocker — GET/HEAD retry is silently disabled (regression from master).** `AbstractHttpClientPlugin.execute` gates retry on body replay: `bodyReplayEnabled = configuredRetryTimes > 0 && isRequestBodyRequired(httpMethod) && isCacheable(exchange)` (AbstractHttpClientPlugin.java:85-87) and then `retryTimes = bodyReplayEnabled ? configuredRetryTimes : 0`. `isRequestBodyRequired` returns false for GET/HEAD (AbstractHttpClientPlugin.java:184-186), so every GET/HEAD with `HTTP_RETRY>0` gets `retryTimes=0` and never retries — across all strategies (current, failover, exponential, fixed, custom). On master, `retryTimes` came straight from `HTTP_RETRY` (master AbstractHttpClientPlugin.java:74) and GET retried fine because `NettyHttpClientPlugin.doRequest` (master :82-87) never subscribes to the body for GET/HEAD, so `retryWhen` resubscribed cleanly. There is no GET-retry test in `RequestBodyReplayRetryTest`. Retry-enabled and body-needs-replay are two separate concerns; please decouple them: keep `retryTimes = configuredRetryTimes` and only use `bodyReplayEnabl ed` to choose between `getCachedRequestBody(exchange)` and `exchange.getRequest().getBody()`. **Should fix — cache cap trusts the forgeable Content-Length, not actual bytes.** `isCacheable` (AbstractHttpClientPlugin.java:130-133) checks `exchange.getRequest().getHeaders().getContentLength() <= maxInMemorySize`, but `getCachedRequestBody` (AbstractHttpClientPlugin.java:152-160) then calls `DataBufferUtils.join(body)`, which aggregates every emitted buffer into one heap buffer with no bound. A client can send `Content-Length: 1` and stream gigabytes, passing the gate while `join` buffers the whole payload on heap — an OOM/DoS on any retry-enabled route. Please enforce the cap against actual accumulated bytes during aggregation (e.g. a bounded join that releases buffers and errors with `DataBufferLimitException` once `readableByteCount` exceeds `maxInMemorySize`), and treat the header as a hint only. **Should fix — binary/source-incompatible constructor removal.** `NettyHttpClientPlugin.java:59` and `WebClientPlugin.java:52` replace the public single-arg constructors with two-arg forms; only the starter is updated. These are public API in a framework jar — downstream code, custom plugins, and tests calling `new NettyHttpClientPlugin(httpClient)` / `new WebClientPlugin(webClient)` will break. Please keep the single-arg constructors as `@Deprecated` delegating overloads (defaulting `maxInMemorySize`). **Should fix — int overflow in `maxInMemorySize * BYTES_PER_MB`.** `HttpClientPluginConfiguration.java:113` and `:134` compute `properties.getMaxInMemorySize() * Constants.BYTES_PER_MB` in `int` (property is `Integer` at HttpClientProperties.java:381, `BYTES_PER_MB` is `int` at Constants.java:811), passed to the `int` field at AbstractHttpClientPlugin.java:64. For `maxInMemorySize >= 2048`, `2048 * 1048576 = 2^31` overflows to negative and `isCacheable` returns false for every body, silently disabling retry for all bodies; `>= 4096` overflows to 0 with the same effect. An operator raising the cap to allow retry for a multi-MB body gets retry disabled everywhere. Please compute in `long` and store the field as `long`. **Nits.** (1) The warn at AbstractHttpClientPlugin.java:88-91 fires on every GET with retry configured and blames body size for a bodyless method — once the blocker is fixed, GET shouldn't warn at all; for POST the message should reflect the actual reason. (2) `getCachedRequestBody` (AbstractHttpClientPlugin.java:165) wraps request bytes with `exchange.getResponse().bufferFactory()`; the request's own factory would be more correct and less fragile given `NettyHttpClientPlugin.doRequest` casts buffers to `NettyDataBuffer` (NettyHttpClientPlugin.java:83-84). Happy to review a follow-up commit addressing the above. -- 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]
