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
Conversation
… request on connection close
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PR eclipse-vertx#6329 fixed an HTTP/1 active-request metrics leak by making
Http1ServerConnection.handleClosed()callhttpMetrics.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'shandleBegin()(and therefore its metric) is deferred until it becomes the response in progress (seeHttp1ServerConnection.handleNext). If the connection closes before that happens,requestInProgress.metric()isnull, and the new code calledhttpMetrics.requestReset(null), which is a real crash for metrics implementations that dereference the metric object (reproduced withFakeHttpServerMetrics, and confirmed to cascade into a second NullPointerException fromHttp1ServerRequest.handleException()sinceresponseis also null for a not-yet-begun request). Netty swallows both exceptions as WARN logs duringchannelInactive, 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 withSocketTimeoutExceptioneven at PR head.Fix
Http1ServerConnection.handleClosed(): only callhttpMetrics.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; addedtestPipelinedRequestNotYetBegunOnClose, which reproduces the pipelining scenario and asserts noNullPointerExceptionis logged during connection teardown.Evidence
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.Http1xTest(503 tests), the metrics test family (171 tests),spotless:check, and the fullvertx-coremodule suite (6974 tests) all pass with zero failures or errors.Limitations
System.errcapture 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.