fix: avoid NPE in outbound TCP stage when upstream finishes before connect (#3255)#3259
Draft
He-Pin wants to merge 1 commit into
Draft
fix: avoid NPE in outbound TCP stage when upstream finishes before connect (#3255)#3259He-Pin wants to merge 1 commit into
He-Pin wants to merge 1 commit into
Conversation
…nnect Motivation: TcpConnectionStage.closeConnectionUpstreamFinished() dereferenced the connection ActorRef without a null check on the half-close-disabled path. For an outbound connection whose upstream finishes before the Connected message arrives, connection is still null, so connection ! Close threw a NullPointerException and failed the stage. The connecting handler also always issued ConfirmedClose (half-close) regardless of the halfClose setting once such an early-finished upstream was detected. Modification: - Hoist a connection eq null guard to the top of closeConnectionUpstreamFinished() so neither close branch dereferences a null connection; the close is deferred until the connection is established, mirroring closeConnectionDownstreamFinished(). - Make the connecting handler honor the half-close setting: send Close when half-close is disabled and ConfirmedClose when enabled. Result: Outbound connections with halfClose=false no longer throw a NullPointerException when the upstream finishes before the connection is established, and the early-finish close path respects the configured half-close semantics. Tests: - stream-tests/testOnly org.apache.pekko.stream.io.TcpSpec (28 passed). Added a directional test that fails with the NPE before the fix. References: Fixes #3255
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.
Motivation
TcpConnectionStage.closeConnectionUpstreamFinished()dereferences theconnection
ActorRefwithout a null check on the full-close path. For anoutbound connection whose upstream finishes before the
Connectedmessage arrives,
connectionis stillnull(it is only assigned whenConnectedis received), soconnection ! Closethrows aNullPointerExceptionand fails the stage.This is easy to hit with a source that completes eagerly:
Source.emptycompletes synchronously during materialization, i.e. before the asynchronous
Connectedreply, soonUpstreamFinishruns whileconnectionisnull.The existing null check only guarded the half-close (
ConfirmedClose) path,leaving the
halfClose = falsepath unprotected.Additionally, the
connectinghandler always issuedConfirmedClose(ahalf-close) regardless of the
halfClosesetting when the upstream hadalready finished before the connection was established — incorrect when
half-close is disabled, because it would keep the read side open instead of
fully tearing the connection down.
Fixes #3255.
Modification
connection eq nullguard to the top ofcloseConnectionUpstreamFinished()so neither close branch can dereferencea null connection. When the connection is not yet established the close is
deferred until it is — this mirrors the structure already used by the
sibling
closeConnectionDownstreamFinished().connectinghandler honor the half-close setting when the upstreamfinished before connect: send
Closewhen half-close is disabled andConfirmedClosewhen it is enabled.reasoning is clear to future readers.
Result
halfClose = falseno longer throw aNullPointerExceptionwhen the upstream finishes before the connection isestablished.
semantics (
Closefor full-close,ConfirmedClosefor half-close).private[stream]/@InternalApiinternals, sothere is no public API or binary-compatibility change (no MiMa filter
required).
Tests
sbt "stream-tests/testOnly org.apache.pekko.stream.io.TcpSpec"→ 28 passed.(
TcpSpec: "not throw a NullPointerException when full-close is requestedand upstream finishes before the connection is established"). It fails
with the exact
NullPointerExceptionbefore the fix (verified bytemporarily reverting the source change) and passes after.
References
Fixes #3255
This is an original contribution to Apache Pekko, made under the Apache
License 2.0 (i.e. the changes are now Apache licensed). No third-party or
Akka-derived code is included.