Release bind parameters of unwritten messages. - #736
Open
SecondPort wants to merge 1 commit into
Open
Conversation
Bind parameter buffers are retained for the Bind message and released from the finally block in Bind.encode(ByteBuf). Messages that never reach the wire were therefore never released and their buffers leaked. ExtendedFlowDelegate created its messages eagerly while assembling the exchange, so the parameters were already retained when the client refused the conversation on a closed connection without ever subscribing the request stream. Create the messages upon subscription so that a request stream that never gets subscribed retains nothing. NettyOutbound.send(...) does not subscribe the encode publisher if the channel is no longer active, so neither Bind.encode(ByteBuf) nor its release ran for a message that was already queued. Dispose the message once the write attempt terminated. Disposal is idempotent and a no-op for messages that got encoded. [resolves pgjdbc#734]
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.
Make sure that:
Issue description
Resolves #734.
ExtendedFlowOperator.getMessages(…)retains the bound parameter buffers for theBindit creates, and that reference is given back only by thefinallyblock inBind.encode(ByteBuf).ExtendedFlowOperator.close(…)returns the operator's own reference fromdoFinally. A buffer therefore reachesrefCnt0 only if the message is also encoded, and any message that never reaches the wire strands one reference per non-null parameter.Two paths do that:
The conversation is refused before the request stream is subscribed. The three
fetch*methods passFlux.just(new CompositeFrontendMessage(factory.createMessages()))as an argument, so the messages — and their retains — are created while the exchange is assembled. WhenClient.exchange(…)refuses on!isConnected(), the request stream is never subscribed,doFinally(operator.close)returns one reference and theBind's reference is stranded.The channel dies while the message is queued.
ChannelOperations.send(Publisher, Predicate)returnsMono.error(AbortedException.beforeSend())on its!channel().isActive()branch without referencingdataStream. Sincemessage.encode(alloc)is a coldMono.fromSupplier, it is never subscribed, soBind.encode(ByteBuf)and itsfinallynever run. The siblingsendObject(Object)callsReactorNetty.safeRelease(message)under the same check — reactor-netty releases what it holds by value, but it cannot release a cold publisher it never subscribed, so ownership stays with the caller.This is the same class of request-side leak as the
CopyDatafix in 68694fc, one level up:FrontendMessage.dispose()already documents that a message discarded before reaching the wire must release its buffers and that disposal is idempotent. These two paths simply did not honour it.A correction to the issue report, which I filed: a bare cancel over a live connection does not leak.
Operators.discardOnCanceldoes not cancel upstream — it keeps draining until the conversation terminates, sodoFinally(operator.close)fires late andBind.encode'sfinallyhas already run by then. Measured:refCntis still 1 immediately afterthenCancel()and 0 only after the connection closes. The leak requires the message never to be encoded; cancellation is the common real-world trigger, because a cancelled chain releases or closes the connection. The issue title is imprecise on that point.Also worth noting:
Client.send(FrontendMessage)does not leak and is unchanged.doSendRequestwraps the message inMono.just(…), soconcatMaptreats it as a scalar andWeakScalarSubscription.cancel()re-discards it into the existingdoOnDiscard(FrontendMessage.class, FrontendMessage::dispose). Theexchange(…)shape is a non-scalarFluxConcatArray, where an already-emitted value is not re-discarded.New Public APIs
None. No signature changes, no new configuration.
Additional context
The change is +12/-4 across two files:
ExtendedFlowDelegate— create the messages upon subscription (Mono.fromSupplier) instead of while assembling the exchange, at the threefetch*sites. Nothing is retained for a request stream that is never subscribed, which covers theexchange(…)refusal,addConversation's subscribe-time re-check and a full conversation queue in one place.ReactorNettyClient— dispose the message once the write attempt terminated.dispose()is idempotent and a no-op for messages that got encoded (Bindguards on itsAtomicBoolean,CopyDataonrefCnt() > 0,CompositeFrontendMessagepropagates, the default is a no-op).I deliberately did not collapse the double ownership by dropping the extra
retain()ingetMessages(…):operator.close(…)fires fromdoFinallyand can run while theBindis still queued, which would hand released buffers toBind.encode(ByteBuf).Tests are unit tests rather than integration tests, because PR CI runs with
-D skipITsand a regression test under*IntegrationTestswould not guard this. Both new classes drive a realReactorNettyClientover a loopback socket — no Docker, no PostgreSQL — and assertrefCnt()directly rather than relying on the sampling leak detector.ReactorNettyClientUnitTests(the class removed in 86ab56d, reinstated) — path 2, plus a control test on a live channel.PostgresqlStatementCancellationUnitTests— both paths end to end throughPostgresqlStatement, plus a control test that cancels over a live connection.The control tests pass without the fix; they are what shows the failing assertions are the bug rather than a harness artefact. Verified on this branch: without the src/main changes the three regression tests fail with
expected: 0 but was: 1and the two control tests pass; with them,./mvnw -B verifyreports 1413 unit tests green.