Skip to content

fix: avoid NPE resetting metrics for a not-yet-begun pipelined HTTP/1 request on connection close - #2

Draft
jaipilot[bot] wants to merge 1 commit into
jaipilot-campaign/vertx-6329-headfrom
jaipilot/pr-1-CqZrtjhZSWbo
Draft

fix: avoid NPE resetting metrics for a not-yet-begun pipelined HTTP/1 request on connection close#2
jaipilot[bot] wants to merge 1 commit into
jaipilot-campaign/vertx-6329-headfrom
jaipilot/pr-1-CqZrtjhZSWbo

Conversation

@jaipilot

@jaipilot jaipilot Bot commented Aug 22, 2026

Copy link
Copy Markdown

Problem

PR eclipse-vertx#6329 fixed an HTTP/1 active-request metrics leak by making Http1ServerConnection.handleClosed() call httpMetrics.requestReset(requestInProgress.metric()) when a connection closes before the current request's body finished. However, when a second (pipelined) request's headers had already been parsed while the previous response was still in progress, that request's handleBegin() (and therefore its metric) is deferred until it becomes the response in progress (see Http1ServerConnection.handleNext). If the connection closes before that happens, requestInProgress.metric() is null, and the new code called httpMetrics.requestReset(null), which is a real crash for metrics implementations that dereference the metric object (reproduced with FakeHttpServerMetrics, and confirmed to cascade into a second NullPointerException from Http1ServerRequest.handleException() since response is also null for a not-yet-begun request). Netty swallows both exceptions as WARN logs during channelInactive, silently defeating the very metrics fix the PR introduced for that pipelined request.

Separately, the PR's own added test (Http1xMetricsLeakTest) read the client socket until EOF, but the server never closes a keep-alive connection on its own in this scenario, so the test hung and failed with SocketTimeoutException even at PR head.

Fix

  • Http1ServerConnection.handleClosed(): only call httpMetrics.requestReset(metric) when the pipelined request's metric is non-null (i.e., it was actually begun). No other logic changed; the original leak scenario (single request, metric begun, body incomplete, connection closed) is unaffected.
  • Http1xMetricsLeakTest: repaired the existing test to read only the response headers (no body expected) before closing the socket, instead of waiting for an EOF that never arrives; added testPipelinedRequestNotYetBegunOnClose, which reproduces the pipelining scenario and asserts no NullPointerException is logged during connection teardown.

Evidence

  • Reproduced the NPE with a raw-socket test before the fix (captured via System.err); confirmed the new regression test fails deterministically without the guard and passes with it.
  • behavior_baseline/behavior_candidate: same command (Http1xMetricsLeakTest) passes 9/9 against original PR-head production code and 10/10 against the final candidate.
  • Broader verification: Http1xTest (503 tests), the metrics test family (171 tests), spotless:check, and the full vertx-core module suite (6974 tests) all pass with zero failures or errors.

Limitations

  • No performance or dependency/version change is included; this is a single defensive-but-necessary null check plus a test fix, bounded to the two files the original PR touched.
  • The regression test detects the NPE via a temporary System.err capture around the raw-socket interaction (Netty logs the swallowed exception to its configured logger); this is a pragmatic, self-contained way to prove the fix without modifying shared test fixtures (FakeHttpServerMetrics) that are outside this PR's scope.

Generated by JAIPilot Cloud for #1 from Anthropic session sesn_01AmbZRGWCkBCqZrtjhZSWbo.

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.

0 participants