[
https://issues.apache.org/jira/browse/THRIFT-6080?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Christopher Tubbs reassigned THRIFT-6080:
-----------------------------------------
Assignee: Christopher Tubbs
> 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
> 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)
