Repository navigation
Conversation
| } | ||
|
|
||
| @Test | ||
| public void testChunkedBodyAtLimitWithDifferentChunkSizes() throws Exception { |
There was a problem hiding this comment.
[NIT] Varying the client chunk size does not guarantee that Jetty will re-intercept the same Content after a partial read. Could we add a test that reads the chunked body one byte at a time to pin down that it is not double-counted?
There was a problem hiding this comment.
[NIT] A one-byte InputStream test would also pin down the counter choice: replacing getContentReceived() with getContentConsumed() still passes the existing 23 tests. In a 64 KiB-limit probe, the consumed variant returned limit + 1 bytes to the servlet before rejecting, while this PR stayed within the limit, and the original handler rejected an at-limit body early. Suggest asserting (1) a body of exactly limit bytes is fully read, and (2) for an oversized chunked body, BadMessageException is thrown and the bytes successfully returned by read() are <= limit.
There was a problem hiding this comment.
@0xbigapple @waynercheung Addressed in 6b9526938e.
Added RequestSizeLimitHandlerTest with one-byte InputStream.read() regressions at a 64 KiB limit:
- An exactly 65536-byte chunked body is fully read without a size exception.
- An oversized chunked body raises
BadMessageExceptionwith code 413, and successful reads return at most 65536 bytes. The test does not require consuming the entire budget before rejection.
Isolated mutation checks confirm that the received-counter implementation passes both tests. Replacing it with getContentConsumed() fails the read-budget assertion (65537 bytes successfully returned); the original Jetty handler fails exact-limit acceptance.
Also added four overlapping mixed CL/chunked requests with a forced first-arrival checkpoint, verifying independent inputs and per-request cumulative counters, plus alternating CL/chunked requests on one keep-alive connection to verify counter reset when the input is reused.
waynercheung
left a comment
There was a problem hiding this comment.
Verified the root cause against Jetty 9.4.58: HttpInput can pass the same Content to the interceptor repeatedly (on arrival and on subsequent reads), and LimitInterceptor adds remaining() each time, so unread bytes are counted more than once. getContentReceived() is incremented once per addContent and reset per request, which avoids this.
Local run (JDK 17): SizeLimitHandlerTest 23/23 and JsonrpcServiceTest#testJsonRpcSizeLimitIntegration pass. Mutants for the original handler, super.handle(), setInterceptor, the Content-Length precheck and the streaming check are all killed. A one-byte-read probe reproduces the early rejection with the original handler and confirms the fix.
No runtime correctness issue found. Please address the Javadoc note and the description fix below before merge.
[SHOULD] Please correct the error-handling row in the description: JsonRpcServlet reads the network body before invoking jsonrpc4j, and oversized chunked requests propagate as HTTP 413, as asserted by testJsonRpcSizeLimitIntegration. HTTP 200 with an error body can still occur in HTTP servlets that swallow the exception; it does not describe the current JSON-RPC body-reading path.
| import org.eclipse.jetty.server.Request; | ||
| import org.eclipse.jetty.server.handler.SizeLimitHandler; | ||
|
|
||
| public class AccurateSizeLimitHandler extends SizeLimitHandler { |
There was a problem hiding this comment.
[SHOULD] Please add class Javadoc documenting the request-only scope and the intentional bypass of super.handle(). Calling super.handle() would reinstall the faulty request interceptor; bypassing it also skips response interception, so adding a response-limit argument alone would not enable response limiting.
There was a problem hiding this comment.
[NIT] Optional: a name such as RequestSizeLimitHandler would make the request-only scope visible at the call site; Accurate does not say how it differs from SizeLimitHandler.
There was a problem hiding this comment.
@waynercheung Addressed in 6b9526938e.
Renamed the class to RequestSizeLimitHandler and updated HttpService and its tests. Class Javadoc now explicitly documents the request-only scope, why super.handle() is intentionally bypassed, and why adding a response-limit argument alone would not install response enforcement.
The PR description now also distinguishes the actual JSON-RPC path: JsonRpcServlet reads the network body before passing the cached body to jsonrpc4j, so oversized chunked requests propagate as HTTP 413. HTTP 200 with an error/empty body is limited to ordinary HTTP servlet paths that catch the streaming exception.
add one-byte read regressions for exact-limit acceptance and oversized request rejection, plus concurrent counter isolation and keep-alive reset coverage. rename the request-only handler and document why its parent handle method is bypassed.
|
Addressed the review feedback in
Validation on the code merged with upstream A pre-merge clean build passed. The pre-merge full test run was interrupted to merge upstream and run the selected tests directly; no completed full-suite pass is claimed for the merged head. @0xbigapple @waynercheung, could you please review the updated commit? Both inline threads have detailed replies. |
| @Override | ||
| public void handle(String target, Request baseRequest, HttpServletRequest request, | ||
| HttpServletResponse response) throws IOException, ServletException { | ||
| if (requestLimit >= 0) { |
There was a problem hiding this comment.
[NIT] replace the long warpper if with below earlier return:
if (requestLimit < 0) { return}
There was a problem hiding this comment.
@SeriousCoding789 Keeping the conditional block here. A negative limit disables size checking, but the request must still be dispatched downstream; a bare return would skip that dispatch. The current structure keeps one shared delegation point through HandlerWrapper.handle().
| * skips the parent's response interceptor, so this handler supports request limits only. | ||
| * Adding a response-limit constructor argument alone would not enable response enforcement. | ||
| */ | ||
| public class RequestSizeLimitHandler extends SizeLimitHandler { |
There was a problem hiding this comment.
[SHOULD] Extending SizeLimitHandler keeps the faulty parent in the type hierarchy even though its handle() is never called. The Javadoc explains why super.handle() must not be used, but nothing in the code prevents it, so a later change (for example, adding response limiting) could reinstall LimitInterceptor with a single super.handle(...) call. The inheritance also brings two small side effects: requestLimit is stored twice (here and in the parent's _requestLimit), and getHandler().handle(...) at line 48 skips the null check that HandlerWrapper.handle() performs.
The only thing reused from the parent appears to be checkRequestLimit(). Extending HandlerWrapper directly might be simpler, and it would take SizeLimitHandler out of this code path entirely while keeping the same 413 status and message:
public class RequestSizeLimitHandler extends HandlerWrapper {
private final long requestLimit;
public RequestSizeLimitHandler(long requestLimit) {
this.requestLimit = requestLimit;
}
@Override
public void handle(String target, Request baseRequest, HttpServletRequest request,
HttpServletResponse response) throws IOException, ServletException {
if (requestLimit >= 0) {
long contentLength = baseRequest.getContentLengthLong();
checkRequestLimit(contentLength);
if (contentLength < 0) {
HttpInput input = baseRequest.getHttpInput();
input.addInterceptor(content -> {
if (content != null) {
checkRequestLimit(input.getContentReceived());
}
return content;
});
}
}
super.handle(target, baseRequest, request, response);
}
private void checkRequestLimit(long size) {
if (size > requestLimit) {
throw new BadMessageException(413,
"Request body is too large: " + size + ">" + requestLimit);
}
}
}With this shape, super.handle() is safe to call, and the Javadoc paragraph about bypassing the parent would no longer be needed.
There was a problem hiding this comment.
@SeriousCoding789 Addressed in 9be048ad1b.
RequestSizeLimitHandler now extends HandlerWrapper directly, owns the request-limit check, and delegates through super.handle(), preserving the null-handler check. The received-byte counter, Content-Length precheck, 413 exception status/message and request-only scope are preserved. The obsolete Javadoc about bypassing the parent has been removed.
The 28 focused Jetty/JSON-RPC tests passed on this commit. Supplemental local checks also confirmed safe handling of an absent downstream handler and normal delegation with negative, zero and positive limits.
| } | ||
|
|
||
| /** POSTs with a specific client chunk size and returns the response body. */ | ||
| private String postChunked(URI uri, byte[] data, int chunkSize) throws Exception { |
There was a problem hiding this comment.
[NIT] Some of the new tests can block instead of failing if the server stops responding. postChunked opens an HttpURLConnection without setConnectTimeout / setReadTimeout (both default to unlimited). postForBody goes through the shared client from HttpClients.createDefault() (line 181), which has no socket timeout. sendRaw (line 645), used by the new HTTP/1.0 and malformed-chunk tests, opens a plain Socket without setSoTimeout. None of the new test methods have @Test(timeout = ...). A method-level timeout, or connect/read timeouts on these clients, would turn a hang into a test failure. RequestSizeLimitHandlerTest already avoids this with setSoTimeout(15000) on its sockets.
There was a problem hiding this comment.
@SeriousCoding789 Addressed in 9be048ad1b.
Added 15-second connect/read timeouts to the HttpURLConnection and raw Socket helpers. The shared Apache HTTP client now also has 15-second connect, connection-pool acquisition and socket timeouts.
The 28 focused Jetty/JSON-RPC tests passed. Supplemental local fault probes invoked the actual three test helpers against loopback servers that accepted connections but never responded; all three raised SocketTimeoutException after approximately 15 seconds. These probes validate read timeouts; connection timeout configuration was checked separately.
| } | ||
|
|
||
| @Test | ||
| public void testHttp10ChunkedRequestCompatibility() throws Exception { |
There was a problem hiding this comment.
[NIT] A few assertions pin Jetty or test-servlet behavior that is independent of the counter fix:
testHttp10ChunkedRequestCompatibility(line 521) checks that Jetty acceptsTransfer-Encoding: chunkedon an HTTP/1.0 request, which HTTP/1.0 does not define.testMalformedChunkFailsDuringBodyRead(line 530) checks Jetty's exception text (Early EOF) and the 200 status produced by the test servlet's catch-all.RequestSizeLimitHandlerTestline 207 (assertSame(inputs.get("/reuse0"), inputs.get("/reuse" + i))) checks that keep-alive requests reuse the sameHttpInputinstance.
If a Jetty upgrade changes any of these, the tests would fail even though the size limit still works. Would it make sense to drop the first two, or label them as documenting current behavior? For the keep-alive test, Assume.assumeTrue(...) on the instance reuse would keep it as a precondition of the scenario rather than a requirement, while the per-request received assertions still verify the counter reset.
There was a problem hiding this comment.
@SeriousCoding789 Keeping these assertions unchanged in this bounded follow-up. The HTTP/1.0 chunked and malformed-input tests record the pinned Jetty version's compatibility/error behavior, rather than a protocol guarantee. Removing or restructuring them is not required for the inheritance/timeout changes.
The same-HttpInput assertion confirms that the keep-alive scenario actually exercises counter reset on a reused input. Replacing it with an assumption could skip that regression coverage. We can reassess these expectations when upgrading Jetty.
SeriousCoding789
left a comment
There was a problem hiding this comment.
[DISCUSS] Pre-existing and outside the scope of this PR, mentioning it only because the chunked path is touched here: the status code for an oversized body still depends on framing and endpoint. Oversized Content-Length requests get 413 before dispatch, and JSON-RPC returns 413 for oversized chunked bodies, but plain HTTP servlets catch the BadMessageException raised during the body read and answer 200 with an error JSON (the behavior testChunkedBodyExceedsLimit documents). A client that branches on the status code sees a success status for a rejected request. This was scoped out in #6658, so a separate issue might be a better place for it than this PR.
extend HandlerWrapper directly and preserve request-limit checks bound test client connection and read waits to 15 seconds
|
@SeriousCoding789 Regarding the HTTP 413 discussion: agreed that uniform status handling should be addressed separately; it is outside this PR. Extending Validation on
These are scoped local checks, not production acceptance or a complete repository test-suite pass. Full-repository test attempts were stopped; no full-suite pass is claimed. Uniform HTTP 413 handling, broader configuration/deployment combinations and sustained resource-pressure testing are outside this follow-up. These supplemental probes are local evidence, not additional committed tests. |
What does this PR do?
Fix premature rejection of valid unknown-length request bodies in the shared Jetty HTTP service. With a 4 MiB limit, the same 2–3 MiB body can pass with Content-Length but fail when sent chunked.
Add
RequestSizeLimitHandlerand install it inHttpService. Keep the known Content-Length precheck; for unknown-length bodies, check Jetty'sHttpInput.getContentReceived()rather than accumulating intercepted buffer sizes. Configured limits and the existing public error policy are preserved.Why are these changes required?
Root cause:
SizeLimitHandler.LimitInterceptorIn Jetty
9.4.58.v20250814,LimitInterceptor.readFrom()performs:The
HttpInput.Interceptorcontract allows the sameContentto pass through an interceptor repeatedly.remaining()is a snapshot of the unread buffer, not newly received bytes. Accumulating it on every invocation counts bytes repeatedly, so the rejection point depends on buffering, chunk boundaries and application read patterns. This affects the unknown-length path; the known Content-Length precheck does not use this counter.Alternatives considered and selected design
Util.checkBodySize()ServletInputStream/ReaderClass Javadoc documents the request-only scope and the intentional bypass of the parent handler.
The replacement preserves existing input interceptors with
addInterceptor()and returns each Content unchanged. It delegates directly togetHandler().handle(...): callingsuper.handle(...)would reinstall the faultyLimitInterceptor. Downstream context, filters and servlets remain in the handler chain.Counter timing and request isolation
Jetty 9.4.58 adds each newly arrived Content's remaining bytes to
_contentArrivedinHttpInput.addContent()before invoking the input interceptor. Updates andgetContentReceived()use the same_inputQmonitor. Re-intercepting an existing Content reads the existing total without incrementing it. Both CL and chunked requests expose bytes received so far; only a complete valid CL body reaches its declared length. Chunked bodies accumulate payload bytes across chunks, excluding framing and trailers.Concurrent HTTP/1 connections have separate HttpInput instances. A keep-alive connection can reuse its HttpInput, but Jetty resets both counters and clears the old interceptor when setting the next request's metadata, before its body arrives. The shared handler stores only the immutable limit; its interceptor captures the current request's input.
This PR has been tested by:
Review follow-up validation (2026-10-29)
The class is renamed from
AccurateSizeLimitHandlertoRequestSizeLimitHandler; class Javadoc explains the request-only scope and why calling the parent handle method is unsafe.Local targeted validation passed 28 tests: the original 23 SizeLimitHandlerTest tests, four new RequestSizeLimitHandlerTest tests, and the real JSON-RPC size-limit integration test. The new tests verify one-byte reads at/over a 64 KiB limit, four overlapping mixed CL/chunked requests with a forced first-arrival checkpoint and distinct HttpInput instances, and four alternating CL/chunked requests on one keep-alive socket with the same HttpInput and fresh counters.
In isolated mutation runs, the received implementation passed both one-byte tests. Replacing it with consumed failed the successful-read budget assertion (65537 bytes returned under a 65536-byte limit); the original Jetty handler failed the exact-limit acceptance assertion. These expected mutant failures demonstrate that the new regressions distinguish both errors.
Merged
upstream/release_v4.8.3atea34f6f0c8before the final focused run. All 30 selected tests passed on the merged code: the 28 request-limit/JSON-RPC tests above, plusManagerTest.testGetCachedTransactionSizeandDbBackfillBloomTest.testEngineSelectionUsesExistingBloomOrTransactionStore, which had failed in the interrupted pre-merge full run. This focused pass does not establish that those intermittent failures are eliminated under full-suite concurrency.All three required module Checkstyle tasks passed after the merge (
:framework:checkstyleMain,:framework:checkstyleTest,:plugins:checkstyleMain). The two one-byte mutation checks were also rerun with the merged dependencies: the received implementation passes; the consumed-counter and original-handler variants fail the expected regression assertions. The pre-mergeclean build -x testpassed. The full test run was stopped at the user's request to merge upstream and test directly; no completed full-suite pass is claimed for this follow-up. Overload/recovery testing was not performed.Reproduction on the unmodified upstream base
On
tronprotocol/java-tron:release_v4.8.3atb70d432eea69c70979a7b4381ccadd57eb4c7745, added only the regression methodtestLargeBodyWithinLimitMatchesContentLengthAndChunkedto the existing test class, without the replacement handler, and ran that single method:With the same 3 MiB body and a 4 MiB limit, the CL response was
{"size":3145728,"bytes":3145728}, while chunked returned{"Error":"BadMessageException"}. The response-equality assertion failed: 1 test completed, 1 failed, Gradle exit code 1. This is an expected regression failure on unmodified production code, not a build/setup failure. The published fix already includes this regression method; the local diagnostic logging used during reproduction is not part of this PR.Completed component test matrix
Historical tests ran on macOS x86_64 / JDK 8 with Jetty 9.4.58 against the pre-rename handler source. Their results remain separate from this follow-up, current-head CI and complete-node validation. The previous independent source hash does not identify the renamed class.
SizeLimitHandlerTest: 23/23 passed, including existing regressions.GetBlockByNumServletwith a mocked Wallet.JsonrpcServiceTest.testJsonRpcSizeLimitIntegrationpassed through JsonRpcServlet/jsonrpc4j.Before this follow-up, local
clean build -x test, module Checkstyle and the full framework suite passed; those full-suite results were not rerun for the renamed class. A PR-base full-project test run encountered a plugins timeout; its isolated retry passed, which is recorded separately from a clean full-project run.Follow up
No additional implementation is proposed as part of this request-byte-counting fix. Response-size enforcement would be a separate feature: if it is introduced, the interception design and its tests must be extended together. The request-only boundary is explained below.
Extra details
Why the broader test suites are not acceptance gates for this fix
HttpServicealready usesresponseLimit = -1; this PR does not enable output enforcement. There is no active response-limit behavior to validate in this change. Positive response limits require a separate implementation and its output-specific tests, not an unfinished task in this fix.ReadListenerconsumptionsetReadListener/startAsyncregistration was found in the production sources checked. Current regression tests exercise the synchronous Reader/InputStream consumers used here; an asynchronous consumer must bring its own integration tests when introduced.HttpService.initContextHandler()installation path; no subclass override of that method was found. Existing per-service limit selection is unchanged. Shared-handler tests, two-service limit isolation and real HTTP/JSON-RPC integration cover the changed mechanism. Repeating identical byte-count tests on every port is not required to validate that shared implementation; this does not claim every service deployment was exercised.Component impact matrix
RequestSizeLimitHandler/HttpInputsize > limitboundary remain. Direct component and socket coverage listed above.HttpServiceUtil.checkBodySize()remains a separate post-read check. Complete configuration-loading validation is not claimed.responseLimitis fixed to -1. See the explicit design boundary below.Request-only scope and future
responseLimitsupportRequestSizeLimitHandlercallssuper(requestLimit, -1)and bypassesSizeLimitHandler.handle(). As a result, it does not install the parent's response-limiting output interceptor. This matches the existingHttpServiceuse, which already disables response limiting, but the subclass is not a general replacement supporting both limits.Supporting a positive
responseLimitlater requires refactoring how request and response interception are installed, for example into an explicitHandlerWrapperwith independently defined input/output enforcement. Merely adding a response-limit argument would leave the output check absent; callingsuper.handle()to restore it would reintroduce the buggy input interceptor. The refactor must preserve existing output interceptors and define counting/error semantics for buffered, streaming and already-committed responses before adding the corresponding test matrix. A separate application JSON-RPC response-size limit is not equivalent to this Jetty output limit.For valid CL requests without a parser conflict,
C > limitrejects before servlet dispatch, but does not guarantee an immediate response on the wire. WhenC <= limit, the current body is framed by C; additional connection bytes are not part of that body and must not be counted toward its limit.