Updated CompositeByteBuf to be responsible for retaining its ByteBufs

[rozza]

• Updated NettyByteBuf so that it does its own reference counting as the internals of Netty can also retain and release the netty ByteBuf implementation.
• Updated CommandMessage as CompositeByteBuf handles the releasing of its ByteBufs
• Migrated CompositeByteBufSpecification to JUnit 5 and added extra test cases with mixed ByteBuf types not just NIO ones.
• Added recording to CommandHelperSpecification as this caught the initial use of Netty doing its own accounting on its ByteBuf implementations.

JAVA-5982

[Copilot]

Pull Request Overview

This PR updates CompositeByteBuf to take responsibility for managing the reference counting lifecycle of its constituent ByteBuf components, rather than relying on external callers to handle this.

• Updated NettyByteBuf to maintain its own reference count separate from the underlying Netty buffer
• Modified CompositeByteBuf to retain/release component buffers during its own lifecycle management
• Migrated test suite from Groovy/Spock to JUnit 5 with enhanced coverage for mixed buffer types

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
CompositeByteBufTest.java	New JUnit 5 test suite with parameterized tests for ByteBufNIO, NettyByteBuf, and mixed buffer scenarios
CompositeByteBufSpecification.groovy	Removed legacy Groovy/Spock test file being replaced by JUnit 5 version
CommandHelperSpecification.groovy	Added recording configuration to enable proper implementation logging
NettyByteBuf.java	Implemented independent reference counting with AtomicInteger to track buffer lifecycle
CompositeByteBuf.java	Updated to retain/release component buffers and duplicate them properly in copy constructor
CommandMessage.java	Simplified buffer management by removing manual release of byte buffers

Comments suppressed due to low confidence (2)

driver-core/src/test/unit/com/mongodb/internal/connection/CompositeByteBufTest.java:1

• The comment in line 246 says 'accross' but should be 'across'.

/*

driver-core/src/test/unit/com/mongodb/internal/connection/CompositeByteBufTest.java:1

• The comment in line 280 says 'accross' but should be 'across'.

/*

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[codeowners-service-app]

Assigned stIncMale for team dbx-java because nhachicha is out of office.

[stIncMale]

I am still reviewing the PR. Despite the changes in the PR being deceivingly small, they should not be taken lightly.

The way reference counting is done in our codebase is notoriously bad, and caused many difficulties maintaining the codebase in the past. The main reason is, of course, the lack of the language mechanisms in Java that would have allowed us to implement reference-counting in a robust way (Rust with its ownership, ownership transfer and borrowing allows for robust reference counting).

[stIncMale]

This assertion combined with the fact that release/retain update referenceCount together with the reference count of each component.buffer, suggests to me that immediately after (in program order) constructing a CompositeByteBuf instance, the reference count of each component.buffer must be equal to referenceCount, which is 1. If this reasoning is correct, we should introduce such an assertion to the constructors of CompositeByteBuf. See #1825 (comment) for the proposed code change.

[stIncMale]

To understand whether removing byteBuffers.forEach(ByteBuf::release) here is correct, it is necessary to understand the reference counting–related semantics of the CompositeByteBuf(List<ByteBuf> buffers) constructor called above. That, in turn, necessitates understanding the reference counting–related semantics of the org.bson.ByteBuf.asReadOnly method, which is "helpfully" not specified.

A reader may omit everything from here and to "TL;DR".

So now it is necessary to figure out / establish the aforementioned semantics of org.bson.ByteBuf.asReadOnly, which requires us to look at all of the implementations (I'll refer to a buffer returned from org.bson.ByteBuf.asReadOnly as "derived", and a buffer on which asReadOnly is called as "parent", as this is how those are called in Netty):

• ByteBufNIO.asReadOnly
  • The derived buffer has its own reference count.
  • The underlying buffer is derived from the parent's underlying buffer, has its own reference count (the strong Java reference).
  • The underlying buffer's reference count is decremented (the strong Java reference is removed) when the overlying buffer's reference count becomes 0; it is not changed in other ways.
    • The above matches the semantics of ByteBufNIO.duplicate, but nothing else.
• NettyByteBuf.asReadOnly
  • [current]
    • The derived buffer shares the reference count with the parent (because it is the same buffer); asReadOnly does not change the count.
    • The underlying buffer shares reference count with the parent's underlying buffer (because it is the same buffer); asReadOnly does not change the count.
    • The underlying buffer shares the reference count with the overlying buffer.
  • [proposed]
    • [same] The derived buffer shares the reference count with the parent (because it is the same buffer); asReadOnly does not change the count.
    • [same] The underlying buffer shares reference count with the parent's underlying buffer (because it is the same buffer); asReadOnly does not change the count.
    • [different] The underlying buffer's reference count is incremented/decremented together with the overlying buffer's reference count.
• CompositeByteBuf.asReadOnly
  • Throws UnsupportedOperationException.

Thus, deriving the semantics of org.bson.ByteBuf.asReadOnly from its implementations is impossible, as they are all different. Assuming that the reference counting–related semantics of org.bson.ByteBuf.asReadOnly and duplicate must be identical to each other⁽¹⁾, let's look at all the implementations of org.bson.ByteBuf.duplicate:

• ByteBufNIO.duplicate
  • Matches the semantics of ByteBufNIO.asReadOnly, but nothing else.
• NettyByteBuf.duplicate
  • [current]
    • The derived buffer shares the reference count with the parent; duplicate increments the count as per the two below items.
    • The underlying buffer is derived from the parent's underlying buffer, shares the reference count with its own parent; duplicate increments the count.
    • The underlying buffer shares the reference count with the overlying buffer.
  • [proposed in the PR]
    • [different] The derived buffer has its own reference count.
    • [same] The underlying buffer is derived from the parent's underlying buffer, shares the reference count with its own parent; duplicate increments the count.
    • [different] The underlying buffer's reference count is incremented/decremented together with the overlying buffer's reference count.
• CompositeByteBuf.duplicate
  • [current]
    • The derived buffer has its own reference count.
    • The underlying buffer (there are multiple) shares reference count with the parent's underlying buffer (because it is the same buffer); duplicate does not change the count.
    • The underlying buffer's reference count is not changed in any ways.
  • [proposed in the PR]
    • [same] The derived buffer has its own reference count.
    • [different] The underlying buffer (there are multiple) is derived from the parent's underlying buffer with semantics of org.bson.ByteBuf.duplicate, which, as we have established, depends on the implementation.
    • [different] The underlying buffer's reference count is incremented/decremented together with the overlying buffer's reference count.

TL;DR

Given the above, I find it virtually impossible to reason about the correctness of both the code before the PR, and the proposed changes. So I added sample tests (I am not suggesting that their current locations is the right one, nor that they should be added to the codebase):

• CompositeByteBufTest.complexTest imitates what's going on in CommandMessage.getCommandDocument;
• CompositeByteBufTest.release/asReadOnly/duplicate/duplicateComposite expose more issues.

Ideally, we should:

1. come up with the reference counting–related semantics of relevant methods of org.bson.ByteBuf;
2. make all implementations adhere to that semantics;
3. update the driver code such that it uses org.bson.ByteBuf and all its implementations in accordance to the established semantics.

I am not insisting on us doing that now, but nor do I see how to approve the current PR: one one hand, it definitely solves something, on the other hand, I have no idea if it breaks something else, and how bad the effect may be. Some sample tests definitely behave differently when run against main, though still fail; this suggests to me that with this PR we may trade some existing bugs to some other bugs.

---

⁽¹⁾ Netty calls such buffers "derived", and they have the following semantics: "a parent buffer and its derived buffers share the same reference count, and the reference count does not increase when a derived buffer is created".

[stIncMale]

Suggested change

	public final class CompositeByteBufTest {
	final class CompositeByteBufTest {

[stIncMale]

This is the assertions I proposed in #1825 (comment) (do not apply it with the button, as GitHub messed something up and I doubt it will apply correctly)

Suggested change

	int thisReferenceCount = referenceCount.get();
	for (ByteBuf cur : buffers) {
	Component component = new Component(cur.asReadOnly().order(ByteOrder.LITTLE_ENDIAN), offset);
	assertTrue(component.buffer.getReferenceCount() == thisReferenceCount);

[stIncMale]

The last reviewed commit is 23475ba.

[rozza]

@stIncMale thanks for the review.

I'm not sure what to do with this as the behaviour is largely undefined, especially the interaction with bufferProviders and ByteBufs. That it makes it hard to reason about.

All we have is the current test cases and interactions that require behaviour to "just work" and the observation that InternalStreamConnection.setRecordEverything(true) causes Netty to error as the underlying proxied byteBuf has already been freed when it comes to getting the command message for logging.

It should also be noted the suggested tests fail on main as well - so in short its very confusing!

[rozza]

Closing and will open a new PR - this approach just opens up to many cans of worms.