Skip to content

fix(http): correct chunked request body size counting - #7074

Open
bladehan1 wants to merge 4 commits into
tronprotocol:release_v4.8.3from
bladehan1:fix/http-chunked-body-size-limit
Open

bladehan1 wants to merge 4 commits into
tronprotocol:release_v4.8.3from
bladehan1:fix/http-chunked-body-size-limit

Conversation

@bladehan1

@bladehan1 bladehan1 commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

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 RequestSizeLimitHandler and install it in HttpService. Keep the known Content-Length precheck; for unknown-length bodies, check Jetty's HttpInput.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.LimitInterceptor

In Jetty 9.4.58.v20250814, LimitInterceptor.readFrom() performs:

_read += content.remaining();
checkRequestLimit(_read);

The HttpInput.Interceptor contract allows the same Content to 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

Option Assessment
Increase the limit or compensate for client chunk size Moves the failure threshold without fixing repeated counting; the error factor is not constant.
Rely only on Util.checkBodySize() Checks after full-body loading and character conversion; cannot provide equivalent streaming protection.
Wrap ServletInputStream / Reader Requires covering both APIs and asynchronous reads; consumption-based counting also changes the metric from received bytes.
Copy the full Jetty handler Adds maintenance of unrelated upstream implementation details.
Upgrade to Jetty 12 A broader JDK/Servlet/API migration rather than a bounded fix.
Use Jetty's received-byte counter Selected: Jetty increments it for newly arrived content and resets it per request. It avoids duplicate counting without another full-body buffer or dependency upgrade.

Class 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 to getHandler().handle(...): calling super.handle(...) would reinstall the faulty LimitInterceptor. 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 _contentArrived in HttpInput.addContent() before invoking the input interceptor. Updates and getContentReceived() use the same _inputQ monitor. 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 AccurateSizeLimitHandler to RequestSizeLimitHandler; 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.3 at ea34f6f0c8 before the final focused run. All 30 selected tests passed on the merged code: the 28 request-limit/JSON-RPC tests above, plus ManagerTest.testGetCachedTransactionSize and DbBackfillBloomTest.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-merge clean build -x test passed. 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.3 at b70d432eea69c70979a7b4381ccadd57eb4c7745, added only the regression method testLargeBodyWithinLimitMatchesContentLengthAndChunked to the existing test class, without the replacement handler, and ran that single method:

./gradlew :framework:test --tests org.tron.common.jetty.SizeLimitHandlerTest.testLargeBodyWithinLimitMatchesContentLengthAndChunked --console=plain

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.

Area / scenarios Result and evidence boundary
Known-length and chunked size boundaries Empty, below limit, exact limit and over-limit cases covered. SizeLimitHandlerTest: 23/23 passed, including existing regressions.
Core premature-rejection regression Same 2/3 MiB bodies under a 4 MiB limit compared across CL/chunked. Independent candidate matrix: 143/143 size assertions matched; original handler prematurely rejected 41 corresponding cases.
Chunking and application reads Multiple chunk sizes, Reader, small/large InputStream reads, UTF-8 and a slow-send sample covered; accepted probe bodies checked against byte counts and SHA-256.
Request lifecycle and component composition Continuous requests, same-connection successful requests, two-service limit isolation and preservation of an existing input interceptor covered. Concurrent overload/recovery is not established.
Zero and disabled handler limits Zero permits empty bodies only; non-empty CL fails before dispatch and chunked fails during reading. Direct handler construction with -1 disables this check; it does not imply node configuration accepts negative values.
CL differs from uploaded bytes Over-limit declarations rejected without servlet dispatch whether no/less/more content was sent. Short bodies produced idle timeout or Early EOF. Extra bytes were parsed as a subsequent request: invalid suffix gave 200 then 400; valid second request gave two 200 responses.
TE/CL conflicts Original and replacement probes returned parser-level 400 without servlet dispatch, including when CL exceeded the limit. The zero-limit/CL/TE edge probe set passed 26/26 assertions across both implementations.
Real HTTP query and JSON-RPC paths 3 MiB CL/chunked requests passed through the real GetBlockByNumServlet with a mocked Wallet. JsonrpcServiceTest.testJsonRpcSizeLimitIntegration passed through JsonRpcServlet/jsonrpc4j.
Protocol and error-path compatibility HTTP/1.0 CL boundaries, HTTP/1.1 CL/chunked, Jetty's HTTP/1.0 chunked compatibility sample, malformed chunk reads, parser 400 and URI 414 covered. Compatibility acceptance is not a standards guarantee.

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

Area Current behavior and reason for the test scope
Response-size boundaries, output streaming and committed-response errors HttpService already uses responseLimit = -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.
Asynchronous ReadListener consumption No setReadListener/startAsync registration 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.
Small reads versus abandoning a body Multiple small InputStream reads are covered, as are short uploads, idle timeout and Early EOF. Application-side abandonment/draining of a body was not separately tested. The fix reads Jetty's received-byte counter without consuming or buffering Content; it does not introduce a drain/abort policy or promise to inspect an upload that the application never reads. A complete drain/abort lifecycle suite is therefore outside this counter-fix acceptance scope.
Starting every production service/port The production subclasses use the same 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.
Complete configuration-loading combinations This PR changes neither configuration parsing/defaults nor the selection of a service's limit. Tests cover the handler's positive/zero/disabled values and independent service limits. Configuration migration and a full loader cross-product are not new behaviors introduced by this fix.
gzip / transformed input No request GzipHandler/GzipFilter is installed in the checked Jetty service chain. There is no decompression path in this change to test. Introducing decompression would require defining and testing both encoded and decoded size limits.
Sustained load, resource exhaustion and recovery Those suites establish broader capacity and resilience claims, not whether repeated interceptor calls count the same bytes twice. This change reuses Jetty's existing per-request counter and adds no full-body buffer, executor or store. Boundary, repeated-request and interceptor-composition tests validate the changed mechanism; long-running pressure/recovery testing is not an acceptance gate for this bounded fix, and no general OOM/DoS guarantee is claimed.
Jetty HTTP/2 The current Jetty HTTP API does not install an HTTP/2 connector. Its body/frame/stream tests are not applicable and are not pending work. gRPC's HTTP/2 transport does not use this handler.

Component impact matrix

Component / boundary Impact and validation scope
RequestSizeLimitHandler / HttpInput Changes unknown-length request accounting to received bytes. CL prechecks and the size > limit boundary remain. Direct component and socket coverage listed above.
Shared HttpService All subclasses using this base handler inherit the replacement: FullNode/Solidity/PBFT HTTP, JSON-RPC HTTP and Admin HTTP. Representative HTTP/JSON-RPC and simulated-service tests passed; this is not a claim that every production port was individually started.
Filters, servlets and existing input interceptors Downstream dispatch is preserved; interceptor-chain and representative servlet tests passed. Complete authentication/rate-limit/filter combinations were not individually exercised.
Parser and connection framing CL framing, malformed-input handling and TE/CL rejection remain Jetty-owned. Timeout/EOF and valid/invalid subsequent-request probes confirm representative behavior.
Configuration and application fallback No keys/defaults are changed. HTTP and JSON-RPC keep their existing per-service limit selection; Util.checkBodySize() remains a separate post-read check. Complete configuration-loading validation is not claimed.
Error handling CL rejection remains pre-dispatch 413. Oversized chunked requests in the current JsonRpcServlet propagate as HTTP 413: the servlet reads the network body before passing the cached body to jsonrpc4j. Some ordinary HTTP servlets catch streaming exceptions and can return HTTP 200 with an error/empty body; this PR does not standardize those responses.
Response limiting Not supported by this request-only replacement; responseLimit is fixed to -1. See the explicit design boundary below.
gRPC, IPC, HTTP/2 and consensus No change to their implementations or limits; gRPC/IPC do not traverse this Jetty request handler. Jetty HTTP/2 is not enabled; no consensus/state-transition logic changes.

Request-only scope and future responseLimit support

RequestSizeLimitHandler calls super(requestLimit, -1) and bypasses SizeLimitHandler.handle(). As a result, it does not install the parent's response-limiting output interceptor. This matches the existing HttpService use, which already disables response limiting, but the subclass is not a general replacement supporting both limits.

Supporting a positive responseLimit later requires refactoring how request and response interception are installed, for example into an explicit HandlerWrapper with independently defined input/output enforcement. Merely adding a response-limit argument would leave the output check absent; calling super.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 > limit rejects before servlet dispatch, but does not guarantee an immediate response on the wire. When C <= 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.

}

@Test
public void testChunkedBodyAtLimitWithDifferentChunkSizes() throws Exception {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 BadMessageException with 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 waynercheung left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
@bladehan1

Copy link
Copy Markdown
Collaborator Author

Addressed the review feedback in 6b9526938e:

  • Added one-byte-read regressions that distinguish the received counter from both the consumed-counter variant and Jetty's original handler.
  • Added concurrent request-isolation and keep-alive counter-reset coverage.
  • Renamed the handler to RequestSizeLimitHandler and documented the request-only scope and intentional parent-handler bypass.
  • Corrected the error-handling row: the current JsonRpcServlet network-body path propagates oversized chunked requests as HTTP 413, before jsonrpc4j is invoked. The existing real JSON-RPC integration test verifies this behavior.

Validation on the code merged with upstream release_v4.8.3 at ea34f6f0c8: 30 selected tests passed (28 request-limit/JSON-RPC regressions plus the two previously failing Manager/plugins tests); all three required module Checkstyle tasks passed. The received implementation passes both one-byte mutation checks, while the consumed-counter and original-handler variants fail their respective regression assertions.

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.

@0xbigapple 0xbigapple left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Override
public void handle(String target, Request baseRequest, HttpServletRequest request,
HttpServletResponse response) throws IOException, ServletException {
if (requestLimit >= 0) {

@SeriousCoding789 SeriousCoding789 Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT] replace the long warpper if with below earlier return:
if (requestLimit < 0) { return}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT] A few assertions pin Jetty or test-servlet behavior that is independent of the counter fix:

  • testHttp10ChunkedRequestCompatibility (line 521) checks that Jetty accepts Transfer-Encoding: chunked on 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.
  • RequestSizeLimitHandlerTest line 207 (assertSame(inputs.get("/reuse0"), inputs.get("/reuse" + i))) checks that keep-alive requests reuse the same HttpInput instance.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 SeriousCoding789 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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
@bladehan1

Copy link
Copy Markdown
Collaborator Author

@SeriousCoding789 Regarding the HTTP 413 discussion: agreed that uniform status handling should be addressed separately; it is outside this PR. Extending HandlerWrapper directly does not change exception propagation: ordinary HTTP servlets can still catch the size exception during body reading and write an error body with HTTP 200. Unifying that behavior requires a separate error-mapping/response policy, including responses that are already committed. This follow-up changes only the inheritance structure and test timeouts, preserving current public error behavior.

Validation on 9be048ad1b:

  • Candidate size matrix: 143/143; original Jetty control: 102/143, with 41 premature chunked rejections.
  • Zero-limit, CL mismatch and TE/CL edge probes: 26/26 across control/candidate.
  • Focused Jetty/JSON-RPC tests: 28/28, zero skipped/failures/errors.
  • One-byte mutation checks: current implementation passes; consumed-counter and original-handler variants fail the expected assertions.
  • Supplemental local probes: absent downstream handler and delegation at negative/zero/positive limits pass; the three actual test helpers time out against nonresponding loopback servers after approximately 15 seconds.
  • clean build -x test, the three CI Checkstyle tasks, and the Math check over tracked Java sources pass.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants