Do various doc and other minor improvements #1157
Conversation
driver-core/src/main/com/mongodb/internal/connection/MessageSettings.java
Show resolved
Hide resolved
| @@ -46,7 +46,7 @@ public interface Stream extends BufferProvider{ | |||
| /** | |||
| * Write each buffer in the list to the stream in order, blocking until all are completely written. | |||
| * | |||
| * @param buffers the buffers to write | |||
| * @param buffers the buffers to write. This method must {@linkplain ByteBuf#release() release} each buffer when it no longer needs it. | |||
There was a problem hiding this comment.
Is this true? SocketStream does not seem to do it.
There was a problem hiding this comment.
It looks like ByteBufferBsonOutput#close, called from InternalStreamConnection#sendCommandMessage, is responsible for the release.
There was a problem hiding this comment.
The apparent deviation from this requirement that you see in SocketStream/AsynchronousChannelStream.write is a result of two peculiarities (I am being generous here):
- Neither
SocketStream.writenorAsynchronousChannelStream.writeuse the buffers they were given after completing (either returning in the synchronous case, or completing the handler in the asynchronous case). So even if the buffers stay unreleased bywrite, the method does not leave a reference to them in the heap memory. This, in turn, means that there may neither be negative effects of aliasing, nor memory leaks caused bywritenot releasing the buffers. However, not releasing may render buffer pooling useless. The latter does not happen due to the next item. SocketStream/AsynchronousChannelStream.writereceive thePooledByteBufNIOimplementation ofByteBufbacked byByteBufNIO, or (I am not sure)CompositeByteBuf. All of theseByteBufimplementations implement theduplicatemethod by creating a new wrappingByteBufwhose reference count is unrelated to the wrapped buffer:duplicate()does not retain the wrapper, andduplicate().release()does not release it. Such implementations are, of course, a recipe for disaster, because it allows the wrapped buffer to be released despite the wrapping buffer still having positive reference count, which may result in the wrapped buffer being aliased.
TL;DR: SocketStream/AsynchronousChannelStream.write do not release buffers, but releasing the buffers they are given is completely pointless anyway due to how they are implemented. Our code here is brittle and seemingly works only due to a lucky coincidence.
The situation with the NettyByteBuf implementation of ByteBuf and NettyStream is different: NettyByteBuf.duplicate does retain the wrapped buffer and the wrapper delegates its retain/release/getReferenceCount to the wrapped buffer. These two things (retaining when wrapping + delegation) guarantees that the wrapper's lifetime is not larger than the wrapped's lifetime. And NettyStream.write actually releases the buffers: "outbound messages are created by your application, and it is the responsibility of Netty to release these after writing them out to the wire".
So, the documentation change for the Stream.write message is correct, but needs a refinement in light of OutputBuffer.getByteBuffers behaving not the way I expected (I checked how it behaves with NettyByteBuf, but the behavior turned out to be different for other ByteBuf implementations). SocketStream/AsynchronousChannelStream violate it and get away with that only because ByteBufNIO and CompositeByteBuf are also broken. The behavior of ByteBufNIO/CompositeByteBuf.duplicate means that I need to fix the doc change for the OutputBuffer.getByteBuffers method (see the new commit).
There was a problem hiding this comment.
I'm still not so sure. Generally when a reference counted object is passed as a parameter, my assumption is that it's the caller's responsibility to release its own reference to to it when the call completes. The receiving object should only release if it also retains, and it doesn't need to retain unless it keeps it for longer than the completion of the call.
For "async" calls it's a bit different in that the above applies not until the async call returns, but until the callback is invoked.
So to my eyes the bug is that InternalStreamConnection calls ByteBufferBsonOutput#getByteBuffers and receives a list of List<ByteBuf> that it, in come cases, does not release.
There was a problem hiding this comment.
I rolled back changes in Stream and created https://jira.mongodb.org/browse/JAVA-5084.
| @@ -100,7 +100,7 @@ default ByteBuf read(int numBytes, int additionalTimeout) throws IOException { | |||
| * Write each buffer in the list to the stream in order, asynchronously. This method should return immediately, and invoke the given | |||
| * callback on completion. | |||
| * | |||
| * @param buffers the buffers to write | |||
| * @param buffers the buffers to write. This method must {@linkplain ByteBuf#release() release} each buffer when it no longer needs it. | |||
There was a problem hiding this comment.
Same as above. AsynchronousChannelStream doesn't seem to do it.
Various things I noticed while working on JAVA-5018.