ByteBuf leak fixes by rozza · Pull Request #1876 · mongodb/mongo-java-driver

rozza · 2026-02-02T15:10:59Z

Ensure Default Server Monitor calls close on resources before interrupt
Update ByteBufferBsonOutput documentation
Improve ReplyHeader testing and ensure resources are closed
Improve ServerSessionPool testing
Ensure reactive client session closing is idempotent
Added System.gc to unified test cleanup. Should cause more gc when testing.

- Ensure Default Server Monitor calls close on resources before interrupt - Update ByteBufferBsonOutput documentation - Improve ReplyHeader testing and ensure resources are closed - Improve ServerSessionPool testing - Ensure reactive client session closing is idempotent - Added System.gc to unified test cleanup. Should cause more gc when testing. JAVA-6081

rozza · 2026-02-02T15:13:54Z

DefaultServerMonitor

Resource cleanup before thread interrupt
Prevents leaks during shutdown scenarios

Other changes

Test migrations
Improved documentation in ByteBufBsonOutput
Improved resource handling in BsonDocument & legacy
Updated reactive stream Fixture to increase use of Netty when transport setting configured
Updated unified tests to increase the gc churn

Copilot

Pull request overview

This PR targets ByteBuf/leak-related issues by tightening resource lifecycle management in core/legacy/reactive code paths and by updating tests/docs to better enforce buffer ownership semantics.

Changes:

Adjust server monitor and reactive session shutdown paths to close/cleanup resources more safely and make session close idempotent.
Replace Spock specifications with JUnit 5 tests for ServerSessionPool and ReplyHeader, and add new monitor edge-case tests.
Clarify/document ByteBuf ownership in ByteBufferBsonOutput and add explicit ByteBuf releasing in BSON serialization code.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java	Adds GC request during unified-test cleanup to help leak detection.
driver-sync/src/test/functional/com/mongodb/client/AbstractSessionsProseTest.java	Avoids assuming `lsid` exists in all commands during sessions prose tests.
driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/Fixture.java	Applies overridden `TransportSettings` to reactive-streams test client settings.
driver-reactive-streams/src/main/com/mongodb/reactivestreams/client/internal/ClientSessionPublisherImpl.java	Makes reactive client session closing idempotent and clears transaction context on close.
driver-legacy/src/main/com/mongodb/DBDecoderAdapter.java	Converts manual closes to try-with-resources for BSON writer/output.
driver-core/src/test/unit/com/mongodb/internal/session/ServerSessionPoolTest.java	New JUnit 5 tests replacing the removed Spock spec.
driver-core/src/test/unit/com/mongodb/internal/session/ServerSessionPoolSpecification.groovy	Removed Spock specification (replaced by JUnit 5 test).
driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerMonitorTest.java	Adds tests for close-during-open and null connection description scenarios.
driver-core/src/test/functional/com/mongodb/internal/connection/ReplyHeaderTest.java	New JUnit 5 tests replacing the removed Spock spec.
driver-core/src/test/functional/com/mongodb/internal/connection/ReplyHeaderSpecification.groovy	Removed Spock specification (replaced by JUnit 5 test).
driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java	Changes monitor shutdown/heartbeat flow to close connections under lock and handle null descriptions.
driver-core/src/main/com/mongodb/internal/connection/ByteBufferBsonOutput.java	Expands ByteBuf ownership documentation and uses `ResourceUtil.release` for safer cleanup.
config/spotbugs/exclude.xml	Adds a SpotBugs exclusion entry for `DefaultServerMonitor`.
bson/src/main/org/bson/BsonDocument.java	Explicitly releases `ByteBuf`s during serialization proxy creation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

nhachicha · 2026-03-10T14:31:39Z

+                return result;
+            });
+
+            if (localConnection != null) {


Condition 'localConnection != null' is always 'true' ?

If close was called twice it would be null on the second iteration

I don't think the double close is possible given that we check for it in https://github.com/rozza/mongo-java-driver/blob/JAVA-6081/driver-core/src/main/com/mongodb/internal/connection/DefaultServer.java#L165-L167. We can keep the check though, if we want to be defensive ...

nhachicha · 2026-03-10T16:09:58Z

                while (!isClosed) {
                    try {
-                        if (connection == null) {
+                        InternalConnection localConnection = connection;


Should we use withLock to dereference the connection?

See previous lock comment

nhachicha · 2026-03-10T16:46:48Z

                    logStateChange(previousServerDescription, currentServerDescription);
                    sdamProvider.get().monitorUpdate(currentServerDescription);

+                    InternalConnection localConnection = connection;


Connection accessed without the lock?

So this use case is fine as its a single snapshot of a volatile here.

I double checked this with claude:

The simple snapshots (L228, L243, L260, L622, L646) are just a single volatile read followed by use of the local. They don't need atomicity with any other field — they just need to avoid reading connection twice. volatile alone is sufficient for that.

So the lock is needed reading a volatile field multiple times without atomicity. Between any two reads, close() or cancelCurrentCheck() on another thread can null the field, causing the second read to return null and NPE on the dereference.

eg Previous Code:

// This contains two read checks and the connection could be nulled inbetween. if (connection == null || connection.isClosed()) { // Do something }

Fix for only do if not closed:

InternalConnection localConnection = withLock(lock, () -> { if (connection == null || connection.isClosed()) { return null; } return connection; }); if (localConnection != null) { // Do something }

Fair enough, although close can only null the connection. cancelCurrentCheck is not

rozza

Responded to comments.

rozza · 2026-03-11T10:00:23Z

                    logStateChange(previousServerDescription, currentServerDescription);
                    sdamProvider.get().monitorUpdate(currentServerDescription);

+                    InternalConnection localConnection = connection;


So this use case is fine as its a single snapshot of a volatile here.

I double checked this with claude:

The simple snapshots (L228, L243, L260, L622, L646) are just a single volatile read followed by use of the local. They don't need atomicity with any other field — they just need to avoid reading connection twice. volatile alone is sufficient for that.

So the lock is needed reading a volatile field multiple times without atomicity. Between any two reads, close() or cancelCurrentCheck() on another thread can null the field, causing the second read to return null and NPE on the dereference.

eg Previous Code:

// This contains two read checks and the connection could be nulled inbetween. if (connection == null || connection.isClosed()) { // Do something }

Fix for only do if not closed:

InternalConnection localConnection = withLock(lock, () -> { if (connection == null || connection.isClosed()) { return null; } return connection; }); if (localConnection != null) { // Do something }

rozza · 2026-03-11T10:01:30Z

                while (!isClosed) {
                    try {
-                        if (connection == null) {
+                        InternalConnection localConnection = connection;


See previous lock comment

rozza requested a review from a team as a code owner February 2, 2026 15:11

rozza requested a review from katcharov February 2, 2026 15:11

rozza requested a review from stIncMale February 2, 2026 15:14

strogiyotec mentioned this pull request Feb 9, 2026

JAVA-6071 #1881

Closed

rozza commented Feb 17, 2026

View reviewed changes

Comment thread driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java

rozza commented Feb 17, 2026

View reviewed changes

Comment thread driver-sync/src/test/functional/com/mongodb/client/AbstractSessionsProseTest.java Outdated

Added gc comment and removed unneeded toBsonDocument in the listener

b6ff5ac

katcharov requested review from nhachicha and removed request for katcharov and stIncMale February 19, 2026 16:31

nhachicha requested a review from Copilot March 10, 2026 12:58

Copilot started reviewing on behalf of nhachicha March 10, 2026 12:59 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

PR review updates

2b92871

rozza requested a review from Copilot March 10, 2026 13:47

Merge branch 'ByteBuf' into JAVA-6081

2df4d98

Copilot started reviewing on behalf of rozza March 10, 2026 13:47 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

PR updates - remove NPE races and release buffers

c6bb3aa

rozza requested a review from Copilot March 10, 2026 14:18

Copilot started reviewing on behalf of rozza March 10, 2026 14:18 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

Comment thread driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerMonitorTest.java

Comment thread driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerMonitorTest.java Outdated

PR updates - copilot recommended changes

9acb046

nhachicha reviewed Mar 10, 2026

View reviewed changes

PR revert sampling logic change - add sample regardless of description

7681b66

rozza commented Mar 11, 2026

View reviewed changes

nhachicha previously approved these changes Mar 24, 2026

View reviewed changes

Merge branch 'ByteBuf' into JAVA-6081

bc48b36

rozza dismissed nhachicha’s stale review via bc48b36 March 24, 2026 15:28

rozza merged commit 8711eef into mongodb:ByteBuf Mar 24, 2026
1 check passed

rozza deleted the JAVA-6081 branch March 24, 2026 15:28

Conversation

rozza commented Feb 2, 2026

Uh oh!

rozza commented Feb 2, 2026

DefaultServerMonitor

Other changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rozza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants