Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"ByteBuf copiedBuffer(ByteBuffer buffer)" in Netty 4.0.24 is NOT thread safe #3896

Closed
seflerZ opened this Issue Jun 18, 2015 · 3 comments

Comments

Projects
None yet
2 participants
@seflerZ
Copy link

seflerZ commented Jun 18, 2015

I'm not sure why this static method is not thread safe, but it bothers me a lot during the development. As the following code shows, the method firstly copy the position out and then set it back after copying. If there are two threads doing the same thing, the position may be stained.

    public static ByteBuf copiedBuffer(ByteBuffer buffer) {
        int length = buffer.remaining();
        if (length == 0) {
            return EMPTY_BUFFER;
        }
        byte[] copy = new byte[length];
        int position = buffer.position();
        try {
            buffer.get(copy);
        } finally {
            buffer.position(position);
        }
        return wrappedBuffer(copy).order(buffer.order());
    }
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Jun 18, 2015

@sefler1987 I think you are right... We should call duplicate on the buffer first. Let me fix it

@normanmaurer normanmaurer self-assigned this Jun 18, 2015

@normanmaurer normanmaurer added this to the 4.0.29.Final milestone Jun 18, 2015

@normanmaurer normanmaurer added the defect label Jun 18, 2015

@seflerZ

This comment has been minimized.

Copy link
Author

seflerZ commented Jun 19, 2015

Thank you so much~

normanmaurer added a commit that referenced this issue Jul 3, 2015

[#3896] Unpooled.copiedBuffer(ByteBuffer) and copiedBuffer(ByteBuffer…
…...) is not thread-safe.

Motivation:

As we modify the position of the passed in ByteBuffer's this methods are not thread-safe.

Modifications:

Duplicate the input ByteBuffers before copy the content to  byte[].

Result:

Unpooled.copiedBuffer(ByteBuffer) and copiedBuffer(ByteBuffer...) is now thread-safe.

normanmaurer added a commit that referenced this issue Jul 7, 2015

[#3896] Unpooled.copiedBuffer(ByteBuffer) and copiedBuffer(ByteBuffer…
…...) is not thread-safe.

Motivation:

As we modify the position of the passed in ByteBuffer's this methods are not thread-safe.

Modifications:

Duplicate the input ByteBuffers before copy the content to  byte[].

Result:

Unpooled.copiedBuffer(ByteBuffer) and copiedBuffer(ByteBuffer...) is now thread-safe.

normanmaurer added a commit that referenced this issue Jul 7, 2015

[#3896] Unpooled.copiedBuffer(ByteBuffer) and copiedBuffer(ByteBuffer…
…...) is not thread-safe.

Motivation:

As we modify the position of the passed in ByteBuffer's this methods are not thread-safe.

Modifications:

Duplicate the input ByteBuffers before copy the content to  byte[].

Result:

Unpooled.copiedBuffer(ByteBuffer) and copiedBuffer(ByteBuffer...) is now thread-safe.

normanmaurer added a commit that referenced this issue Jul 7, 2015

[#3896] Unpooled.copiedBuffer(ByteBuffer) and copiedBuffer(ByteBuffer…
…...) is not thread-safe.

Motivation:

As we modify the position of the passed in ByteBuffer's this methods are not thread-safe.

Modifications:

Duplicate the input ByteBuffers before copy the content to  byte[].

Result:

Unpooled.copiedBuffer(ByteBuffer) and copiedBuffer(ByteBuffer...) is now thread-safe.
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Jul 7, 2015

Fixed by #3930

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.