Repository navigation
GH-1464: Clear vectors before loading their buffers - #1465
Conversation
loadFieldBuffers released the vector's validity buffer before BitVectorHelper.loadValidityBuffer created the new one. When that failed, for example on an allocation failure, the vector still pointed at the released buffer, and closing it later released the buffer again. Create the new validity buffer first in every vector that loads one. loadValidityBuffer also treated a missing validity buffer with an unknown null count (-1) as the mixed case, and kept the null or empty source buffer: a NullPointerException for a NULL C Data buffer, or a vector whose validity buffer is too small. Treat it as all valid, and reject a missing validity buffer when the null count says that only some values are null.
| validityBuffer = BitVectorHelper.loadValidityBuffer(fieldNode, bitBuffer, allocator); | ||
| validityBuffer = newValidityBuffer; | ||
| valueBuffer.getReferenceManager().release(); | ||
| valueBuffer = dataBuffer.getReferenceManager().retain(dataBuffer, allocator); |
There was a problem hiding this comment.
Shouldn't we be doing the same thing for the value buffer? Otherwise you still have an inconsistent vector.
Also, this would mean that you temporarily have 2x memory usage instead of 1x as before. Is it necessarily wrong to say that failing a load means the vector is cleared? After all, if you're loading data, presumably you didn't care about the previous contents of the vector anymore.
There was a problem hiding this comment.
Good points, thanks. You're right: the offset, size and value buffers had the same problem (retain can still throw, for example for a buffer from another root allocator), and there's no reason to keep the old contents of a vector we're loading into.
I changed loadFieldBuffers to release the vector's own buffers and reset its value count before loading anything. A failed load now leaves the vector empty but closeable, with no extra allocation. The leaf vectors just call clear(), as BaseVariableWidthViewVector already did. The list and struct vectors only release their own buffers, so their child vectors are left alone.
I also added a test where retaining a buffer after the validity buffer fails. On the previous commit it hit RefCnt has gone negative on close for every vector except BaseVariableWidthViewVector, which instead leaked the new validity buffer, so I reverted that one to main.
Instead of loading the new validity buffer before releasing the old one, release the vector's own buffers first and reset its value count, then load. A failure while loading the validity buffer, or while retaining any other buffer, now leaves the vector empty but usable, without the extra memory of keeping the old validity buffer. List, list view, fixed size list and struct vectors keep their child vectors. BaseVariableWidthViewVector already cleared itself first: restore it, since loading its validity buffer before clear() leaked that buffer when retaining the view buffer failed.
jbonofre
changed the title
| // Release this vector's buffers first, so that a failure while loading leaves it empty but | ||
| // usable. Unlike clear(), this keeps the child vectors. | ||
| validityBuffer = releaseBuffer(validityBuffer); | ||
| offsetBuffer = releaseBuffer(offsetBuffer); |
There was a problem hiding this comment.
I guess this runs into the question of whether an empty offset buffer is valid for a list vector...but it's consistent with before so we can defer that discussion.
What's Changed
Found while reviewing #1422. This change is independent of it and based on
main.Clear a vector before loading its buffers. Every
loadFieldBuffersreleased the vector's current buffers one at a time, each just before loading its replacement. If one of those loads failed (for exampleOutOfMemoryExceptionwhile creating the validity buffer, orIllegalStateExceptionwhile retaining a buffer from another root allocator), the vector kept pointing at a released buffer, and closing it failed withRefCnt has gone negative. The vector's own buffers are now released and replaced with empty buffers, and its value count is reset, before anything is loaded. This applies toBaseFixedWidthVector,BaseVariableWidthVector,BaseLargeVariableWidthVector,ListVector,LargeListVector,ListViewVector,LargeListViewVector,FixedSizeListVectorandStructVector, so a failed load leaves the vector empty but usable. The leaf vectors callclear(), asBaseVariableWidthViewVectoralready did. The list, list view, fixed size list and struct vectors release only their own buffers, so their child vectors are kept.Handle a missing validity buffer in
BitVectorHelper.loadValidityBuffer.-1, a missing (NULL or empty) validity buffer is now read as all valid, like null count0. Before, it was kept as is: aNullPointerExceptionfor a NULL C Data buffer, or an empty validity buffer for IPC thatisNull(i)then read past.IllegalArgumentExceptionwhen the null count says that only some values are null, instead of being kept.Tests:
TestLoadFieldBufferscovers all ten vector types with a failed validity load, and the eight with more than one buffer with a failed buffer retain. Each time, the vector is left with a value count of 0 and closes with no double release or leak.TestBitVectorHelpercovers the twoloadValidityBuffercases. The fullvectortest suite passes.Closes #1464.