[jira] [Commented] (CASSANDRA-13680) readBytes needs to clone its data

["Jason Brown (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CASSANDRA-13680?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16146358#comment-16146358 ] 

Jason Brown commented on CASSANDRA-13680:
-----------------------------------------

{{ByteBufferUtils#readBytes}} isn't necessarily broken or buggy, as long as the callers are aware that the returned {{ByteBuffer}} has a shared view (and as long as they don't release any direct buffers). It looks like this method was created in March 2014 (not attached to any jira), so it's been in the code base for 3.5 years and is reasonably stable. (note: the code was around before that date; apparently moved from somewhere else in the code base).

Do you have any stack traces or example use cases in which this code is failing? 

CASSANDRA-3179 fixed a specific bug on a class that was explicitly for reading from files: {{MappedFileDataInput}}. So, while the code may be similar, the use and intent are quite different. Furthermore, making {{ByteBufferUtils#readBytes}} is completely unnecessary.

> readBytes needs to clone its data
> ---------------------------------
>
>                 Key: CASSANDRA-13680
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-13680
>             Project: Cassandra
>          Issue Type: Bug
>          Components: Core
>            Reporter: Hao Zhong
>
> The code of the ByteBufferUtil_readBytes method is as follow:
> {code}
>  public static ByteBuffer readBytes(ByteBuffer bb, int length)
>     {
>         ByteBuffer copy = bb.duplicate();
>         copy.limit(copy.position() + length);
>         bb.position(bb.position() + length);
>         return copy;
>     }
> {code}
> I found that CASSANDRA-3179 fixed a related bug. The buggy code is as follow:
> {code}
>  public synchronized ByteBuffer readBytes(int length) throws IOException
>     {
>         int remaining = buffer.remaining() - position;
>         if (length > remaining)
>             throw new IOException(String.format("mmap segment underflow; remaining is %d but %d requested",
>                                                 remaining, length));
>         ByteBuffer bytes = buffer.duplicate();
>         bytes.position(buffer.position() + position).limit(buffer.position() + position + length);
>         position += length;
>         return bytes;
>     }
> {code}
> The fixed code is:
> {code}
>  public synchronized ByteBuffer readBytes(int length) throws IOException
>     {
>         int remaining = buffer.remaining() - position;
>         if (length > remaining)
>             throw new IOException(String.format("mmap segment underflow; remaining is %d but %d requested",
>                                                 remaining, length));
>         if (length == 0)
>             return ByteBufferUtil.EMPTY_BYTE_BUFFER;
>         ByteBuffer bytes = buffer.duplicate();
>         bytes.position(buffer.position() + position).limit(buffer.position() + position + length);
>         position += length;
>         // we have to copy the data in case we unreference the underlying sstable.  See CASSANDRA-3179
>         ByteBuffer clone = ByteBuffer.allocate(bytes.remaining());
>         clone.put(bytes);
>         clone.flip();
>         return clone;
>     }
> {code}
> The ByteBufferUtil_readBytes method may be modified in the same way to handle the similar problem. 



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org