Skip to content

Commit b0472b2

Browse files
sandeshkr419Sandesh Kumar
authored andcommitted
Fix C Data import leaking the whole array on a mid-import allocator OOM
ReferenceCountedArrowArray.unsafeAssociateAllocation retained the imported array's shared reference count *before* calling wrapForeignAllocation, which can throw OutOfMemoryException when the tracking (importing) allocator is over its limit. The retain was not rolled back on that throw, so the reference count stayed permanently elevated: release() never reached zero, the producer's C Data release callback was never invoked, and the entire imported array's buffers leaked in the producer's memory (invisible to the JVM heap). This strands whole batches when importing a wide/variable-width array into a bounded allocator that fills part-way through the array. Retain only after wrapForeignAllocation succeeds, so each retain() is paired with exactly one ForeignAllocation.release0(). If it throws, no ForeignAllocation was created, so skipping the retain is correct: the initial reference held by ArrayImporter.importArray (released in its finally) then drives the count to zero and fires the release callback, freeing the array. Adds ImportOutOfMemoryTest, which exports a multi-buffer batch from a dedicated producer allocator and imports it into an allocator too small to hold it; it asserts the import throws an allocator OOM and that the producer drains to zero (release callback fired). The test fails on the previous retain-before-wrap code (producer strands ~1 MB) and passes with this change.
1 parent 7f1f9f5 commit b0472b2

2 files changed

Lines changed: 154 additions & 7 deletions

File tree

c/src/main/java/org/apache/arrow/c/ReferenceCountedArrowArray.java

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -64,13 +64,18 @@ void release() {
6464
*/
6565
ArrowBuf unsafeAssociateAllocation(
6666
BufferAllocator trackingAllocator, long capacity, long memoryAddress) {
67+
// Retain AFTER wrap: wrapForeignAllocation throws OutOfMemoryException when the allocator is
68+
// over its limit, and a retain() before that throw would leave the count elevated with no
69+
// matching release0(), preventing the array's release callback from ever firing.
70+
ArrowBuf buf =
71+
trackingAllocator.wrapForeignAllocation(
72+
new ForeignAllocation(capacity, memoryAddress) {
73+
@Override
74+
protected void release0() {
75+
ReferenceCountedArrowArray.this.release();
76+
}
77+
});
6778
retain();
68-
return trackingAllocator.wrapForeignAllocation(
69-
new ForeignAllocation(capacity, memoryAddress) {
70-
@Override
71-
protected void release0() {
72-
ReferenceCountedArrowArray.this.release();
73-
}
74-
});
79+
return buf;
7580
}
7681
}
Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
package org.apache.arrow.c;
18+
19+
import static org.junit.jupiter.api.Assertions.assertEquals;
20+
import static org.junit.jupiter.api.Assertions.assertThrows;
21+
import static org.junit.jupiter.api.Assertions.assertTrue;
22+
23+
import java.util.ArrayList;
24+
import java.util.List;
25+
import org.apache.arrow.memory.BufferAllocator;
26+
import org.apache.arrow.memory.OutOfMemoryException;
27+
import org.apache.arrow.memory.RootAllocator;
28+
import org.apache.arrow.vector.FieldVector;
29+
import org.apache.arrow.vector.VarCharVector;
30+
import org.apache.arrow.vector.VectorSchemaRoot;
31+
import org.apache.arrow.vector.types.pojo.Schema;
32+
import org.junit.jupiter.api.AfterEach;
33+
import org.junit.jupiter.api.BeforeEach;
34+
import org.junit.jupiter.api.Test;
35+
36+
/**
37+
* Regression test: a mid-import {@link OutOfMemoryException} must not leak the imported array.
38+
*
39+
* <p>A "producer" allocator owns the exported batch; if the C Data release callback fires, the
40+
* producer drains to zero. A too-small consumer allocator forces an OOM part-way through the
41+
* import. The test asserts the producer drains — confirming the release callback fired despite
42+
* the failure.
43+
*/
44+
final class ImportOutOfMemoryTest {
45+
private static final int ROWS = 1024;
46+
private static final int VALUE_BYTES = 256;
47+
private static final int COLUMNS = 4;
48+
// Far smaller than the exported batch, so the import OOMs part-way through the buffers.
49+
private static final long TINY_LIMIT = 16 * 1024;
50+
51+
private RootAllocator root;
52+
53+
@BeforeEach
54+
public void setUp() {
55+
root = new RootAllocator(Long.MAX_VALUE);
56+
}
57+
58+
@AfterEach
59+
public void tearDown() {
60+
root.close();
61+
}
62+
63+
@Test
64+
public void importOomDoesNotLeakExportedArray() {
65+
// "producer" owns only the exported batch buffers; the C Data struct containers live on a
66+
// separate allocator (they are consumed/closed by import, which would otherwise muddy the
67+
// producer's balance). So producer draining to zero is an exact signal that the array's release
68+
// callback fired.
69+
BufferAllocator producer = root.newChildAllocator("producer", 0, Long.MAX_VALUE);
70+
BufferAllocator structs = root.newChildAllocator("structs", 0, Long.MAX_VALUE);
71+
try (ArrowArray array = ArrowArray.allocateNew(structs);
72+
ArrowSchema schema = ArrowSchema.allocateNew(structs)) {
73+
exportBatch(producer, array, schema);
74+
assertTrue(
75+
producer.getAllocatedMemory() > 0, "producer holds the exported batch before import");
76+
77+
// A consumer allocator far too small to hold the batch: the import throws part-way through.
78+
BufferAllocator consumer = root.newChildAllocator("consumer", 0, TINY_LIMIT);
79+
try (CDataDictionaryProvider provider = new CDataDictionaryProvider()) {
80+
Schema importSchema = Data.importSchema(consumer, schema, provider);
81+
try (VectorSchemaRoot importRoot = VectorSchemaRoot.create(importSchema, consumer)) {
82+
Exception thrown =
83+
assertThrows(
84+
Exception.class,
85+
() -> Data.importIntoVectorSchemaRoot(consumer, array, importRoot, provider));
86+
assertTrue(
87+
hasOutOfMemoryCause(thrown),
88+
"mid-import failure must be an allocator OOM: " + thrown);
89+
}
90+
}
91+
consumer.close();
92+
93+
// The array's release callback must have fired despite the mid-import OOM, freeing the whole
94+
// exported batch. On the unfixed retain-before-wrap code the batch is stranded instead.
95+
assertEquals(
96+
0L,
97+
producer.getAllocatedMemory(),
98+
"import OOM leaked the exported batch (producer not drained)");
99+
}
100+
// Closing these (and the RootAllocator in tearDown) additionally asserts nothing leaked at all.
101+
producer.close();
102+
structs.close();
103+
}
104+
105+
/** True if {@code t} is, or is caused by, an Arrow {@link OutOfMemoryException}. */
106+
private static boolean hasOutOfMemoryCause(Throwable t) {
107+
for (Throwable cause = t; cause != null; cause = cause.getCause()) {
108+
if (cause instanceof OutOfMemoryException) {
109+
return true;
110+
}
111+
}
112+
return false;
113+
}
114+
115+
/**
116+
* Builds a wide multi-column VarChar batch on {@code alloc} and exports it into the C structs.
117+
*/
118+
private void exportBatch(BufferAllocator alloc, ArrowArray array, ArrowSchema schema) {
119+
byte[] value = new byte[VALUE_BYTES];
120+
for (int i = 0; i < value.length; i++) {
121+
value[i] = (byte) 'x';
122+
}
123+
List<FieldVector> vectors = new ArrayList<>(COLUMNS);
124+
for (int c = 0; c < COLUMNS; c++) {
125+
VarCharVector vector = new VarCharVector("col" + c, alloc);
126+
vector.allocateNew((long) ROWS * VALUE_BYTES, ROWS);
127+
for (int r = 0; r < ROWS; r++) {
128+
vector.setSafe(r, value);
129+
}
130+
vector.setValueCount(ROWS);
131+
vectors.add(vector);
132+
}
133+
try (VectorSchemaRoot source = new VectorSchemaRoot(vectors)) {
134+
long total = 0;
135+
for (FieldVector vector : source.getFieldVectors()) {
136+
total += vector.getBufferSize();
137+
}
138+
assertTrue(total > TINY_LIMIT, "test setup: batch must exceed the consumer limit");
139+
Data.exportVectorSchemaRoot(alloc, source, null, array, schema);
140+
}
141+
}
142+
}

0 commit comments

Comments
 (0)