[jira] [Commented] (THRIFT-6080) Java binary types generate getters that incorrectly make redundant array copies after resizing the array from the ByteBuffer source

["Sylwester Lachiewicz (Jira)" <ji...@apache.org>]

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

Sylwester Lachiewicz commented on THRIFT-6080:
----------------------------------------------

Still present on master. In [t_java_generator.cc|https://github.com/apache/thrift/blob/master/compiler/cpp/src/thrift/generate/t_java_generator.cc#L2619] the {{byte[] getX()}} of a binary field calls {{setX(TBaseHelper.rightSize(x))}}, and the generated setter assigns {{TBaseHelper.copyBinary(x)}}, which always allocates a new array. So each call copies the bytes at least once, and twice when the buffer is not right-sized. Apache Accumulo rewrites the generated call with {{sed}} into a direct assignment ([generate-thrift.sh|https://github.com/apache/accumulo/blob/main/core/src/main/scripts/generate-thrift.sh]), which is the fix this ticket describes.

> Java binary types generate getters that incorrectly make redundant array copies after resizing the array from the ByteBuffer source
> -----------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: THRIFT-6080
>                 URL: https://issues.apache.org/jira/browse/THRIFT-6080
>             Project: Thrift
>          Issue Type: Bug
>          Components: Java - Compiler, Java - Library
>            Reporter: Christopher Tubbs
>            Assignee: Christopher Tubbs
>            Priority: Major
>
> In the Java generator, for binary types, two getters are created, one that looks like:
>     byte[] getFieldname();
> And one that looks like:
>     ByteBuffer bufferForFieldname();
> The first method seems to take the underlying bytes for the field, truncate it to size, making a copy if needed, using TBaseHelper.rightSize(ByteBuffer). Then, it makes an additional copy by calling the public API setFieldname(ByteBuffer), which internally uses TBaseHelper.copyBinary(ByteBuffer). All of this appears to be an attempt to create a protective copy on read of the underlying byte array. However, what it actually returns is the actual underlying array of the currently set ByteBuffer. The next time the method is called, it will make another copy of the ByteBuffer that is wrapping the array that was leaked outside the method the last time it was called. This is not correct behavior if it is trying to make a protective copy on read, and it is not correct behavior if it is trying to merely truncate once and always return that same byte array. Neither possible behavior is correct, so it makes no sense to keep making copies on each read.
> The second method appears to be designed to provide direct access to the underlying ByteBuffer, presumably for efficiency if a copy of the byte array is not needed. However, it also makes a protective copy of the bytes and creates a new ByteBuffer object every time, by calling TBaseHelper.copyBinary(ByteBuffer) each time. It appears that this was previously identified as a problem and a flag was added (unsafe_binaries) to the Java generator to give direct access to the underlying ByteBuffer without making a protective copy. However, the problem with that flag is that it removes protective copies everywhere, including in the constructor and in the setters, rather than just give direct access to the underlying ByteBuffer. So, that flag makes it harder for calling code to know when it needs to make its own protective copy and when it doesn't.
> This was identified as a problem in [https://github.com/apache/accumulo/pull/6455] ; after a few failed attempts to work around the problems, we chose to avoid the expensive bufferForFieldname() methods and the unsafe use of unsafe_binaries and patch the generated code at build time in [https://github.com/apache/accumulo/pull/6459] to avoid the problems with unnecessary copies of the array in the getter that returns an array, by setting the internal field to the output of the TBaseHelper.rightSize(ByteBuffer) method instead of setting it via the setFieldname(ByteBuffer) method that makes the unnecessary copy. This still gives callers access to the shared byte array instead of making a protective copy of it each read, though. However, since it was already leaking the byte array, and we are not modifying it, we found this acceptable behavior.
> Ideally, the getter should be modified as we did in [https://github.com/apache/accumulo/pull/6459,] but the array that is returned should be returned with .clone() so it doesn't leak the original array after resizing. This will potentially result in two copies the first time, and only one copy each subsequent time. The alternative is to never modify the underlying ByteBuffer, and just always make a copy each time, and never call setFieldname(ByteBuffer). I think the unsafe_binaries flag is somewhat worthless at this point... I think the entire purpose of having that method is to get access to the underlying ByteBuffer directly. It makes no sense to be generated at all, if it's going to make a protective copy. If it were going to do that, it should just get the protective copy from the standard getter (once fixed properly) and users can wrap it themselves with ByteBuffer.wrap(byte[]) if they really want to. The bufferFor methods should not be created at all by default, unless requested to generate it using a new flag to be added for that purpose. And when it is generated, it will always be generated without the protective copy, because that's its entire reason for existing. Its behavior should not be affected by the 'unsafe_binaries' flag at all. That flag (which I recommend nobody ever use) should only affect the constructor and setters and the getter that returns an array.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)