Copilot commented on code in PR #1240: URL: https://github.com/apache/arrow-java/pull/1240#discussion_r3618697476
########## c/src/test/java/org/apache/arrow/c/ImportOutOfMemoryTest.java: ########## @@ -0,0 +1,142 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.arrow.c; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.List; +import org.apache.arrow.memory.BufferAllocator; +import org.apache.arrow.memory.OutOfMemoryException; +import org.apache.arrow.memory.RootAllocator; +import org.apache.arrow.vector.FieldVector; +import org.apache.arrow.vector.VarCharVector; +import org.apache.arrow.vector.VectorSchemaRoot; +import org.apache.arrow.vector.types.pojo.Schema; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Regression test: a mid-import {@link OutOfMemoryException} must not leak the imported array. + * + * <p>A "producer" allocator owns the exported batch; if the C Data release callback fires, the + * producer drains to zero. A too-small consumer allocator forces an OOM part-way through the + * import. The test asserts the producer drains, confirming the release callback fired despite + * the failure. + */ +final class ImportOutOfMemoryTest { + private static final int ROWS = 1024; + private static final int VALUE_BYTES = 256; + private static final int COLUMNS = 4; + // Far smaller than the exported batch, so the import OOMs part-way through the buffers. + private static final long TINY_LIMIT = 16 * 1024; + + private RootAllocator root; + + @BeforeEach + public void setUp() { + root = new RootAllocator(Long.MAX_VALUE); + } + + @AfterEach + public void tearDown() { + root.close(); + } + + @Test + public void importOomDoesNotLeakExportedArray() { + // "producer" owns only the exported batch buffers; the C Data struct containers live on a + // separate allocator (they are consumed/closed by import, which would otherwise muddy the + // producer's balance). So producer draining to zero is an exact signal that the array's release + // callback fired. + BufferAllocator producer = root.newChildAllocator("producer", 0, Long.MAX_VALUE); + BufferAllocator structs = root.newChildAllocator("structs", 0, Long.MAX_VALUE); + try (ArrowArray array = ArrowArray.allocateNew(structs); + ArrowSchema schema = ArrowSchema.allocateNew(structs)) { + exportBatch(producer, array, schema); Review Comment: The child allocators (`producer`/`structs`/`consumer`) are not protected by try-with-resources/finally. If any assertion or the import call throws before the explicit `close()` calls at the end of the test, `tearDown()` will close the `RootAllocator` with outstanding children and throw, which can mask the real failure. Consider using try-with-resources (or a `finally`) to always close all three allocators even when the test fails early. ########## c/src/main/java/org/apache/arrow/c/ReferenceCountedArrowArray.java: ########## @@ -64,13 +64,18 @@ void release() { */ ArrowBuf unsafeAssociateAllocation( BufferAllocator trackingAllocator, long capacity, long memoryAddress) { + // Retain AFTER wrap: wrapForeignAllocation throws OutOfMemoryException when the allocator is + // over its limit, and a retain() before that throw would leave the count elevated with no + // matching release0(), preventing the array's release callback from ever firing. + ArrowBuf buf = + trackingAllocator.wrapForeignAllocation( Review Comment: `wrapForeignAllocation()` may invoke `ForeignAllocation.release0()` when it throws from inside its internal try/catch (see `BaseAllocator.wrapForeignAllocation`), but this method now calls `retain()` only after `wrapForeignAllocation()` returns. That means a mid-method failure can decrement the refcount (via `release0() -> release()`) without a matching retain, and then `ArrayImporter.importArray`'s `finally` will `release()` again (refcount can go negative, masking the original failure). A safer pattern is to `retain()` before wrapping, and explicitly balance the retain only for the allocator-limit `OutOfMemoryException` path (which is thrown before the `ForeignAllocation` is associated, so `release0()` is not called). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
