Repository navigation
fix(jni): release the schema export in VortexWriter.build() - #18
Conversation
build() closed the exported ArrowSchema without calling its release callback. Native code only borrows the schema, so the child structs the export allocated were never freed, and every writer's allocator closed with outstanding memory. Signed-off-by: gitRasheed <[email protected]>
ChristopherWirt
left a comment
ChristopherWirt
left a comment
There was a problem hiding this comment.
Fix looks correct: Schema::try_from(&*ptr) copies the schema, so releasing after create/createStream is safe, and the release != 0 guard covers a failed export.
One gap outside the diff: native writeBatch also only borrows the schema (&*(arrow_schema_addr as *const FFI_ArrowSchema)), and the documented usage (e.g. JNIWriterTest L264-268, NativeIOBridgeTest) closes it without calling release(). That leaks the schema's children on every batch.
| } | ||
| return new VortexWriter(pointer); | ||
| } finally { | ||
| // Native code only borrows the schema, so the children the export allocated are ours to release. |
There was a problem hiding this comment.
Alternative: create/createStream use the schema once, so native could take ownership with FFI_ArrowSchema::from_raw (as writeBatch already does for the array). Then plain close() on the Java side is always correct, and new call sites can't reintroduce this leak.
| return new VortexWriter(pointer); | ||
| } finally { | ||
| // Native code only borrows the schema, so the children the export allocated are ours to release. | ||
| if (ffi.snapshot().release != 0) { |
There was a problem hiding this comment.
VortexDataWriter.closeArrowResources does the same thing but calls release() unguarded, which breaks if exportSchema threw. Worth a shared releaseAndClose(ArrowSchema) helper used in both places?
| } finally { | ||
| // Native code only borrows the schema, so the children the export allocated are ours to release. | ||
| if (ffi.snapshot().release != 0) { | ||
| ffi.release(); |
There was a problem hiding this comment.
If release() throws, close() is skipped and the original exception from create is lost. Suggest try { ffi.release(); } finally { ffi.close(); }.
| } | ||
|
|
||
| @Test | ||
| public void testCreateWriterReleasesSchemaExport() throws IOException { |
There was a problem hiding this comment.
This only covers the URI path with a flat schema. Consider adding createStream, a nested schema, and a writeBatch call. The last one would fail today because of the writeBatch schema leak.
| public void testCreateWriterReleasesSchemaExport() throws IOException { | ||
| String writePath = tempDir.resolve("test_schema_release.vortex").toUri().toString(); | ||
| Session session = Session.create(); | ||
| try (BufferAllocator allocator = new RootAllocator()) { |
There was a problem hiding this comment.
nit: ArrowAllocation.rootAllocator().newChildAllocator("test", 0, Long.MAX_VALUE) gives the same zero-leak check without a second root allocator.
Signed-off-by: Christopher Wirt <[email protected]>
Preserve the generic Arrow field writers from #17 and exception-safe schema cleanup and regression coverage from #18 alongside independent JNI buffer ownership. Signed-off-by: Christopher Wirt <[email protected]>
Develop changed old-connector files that this branch removes (#17) and the JNI writer files this branch already carries with the new writer port (#18, #49). The resolution keeps this branch's versions and takes the broader schema release test from #18. Signed-off-by: gitRasheed <[email protected]>
Summary
VortexWriter.build()exports an Arrow schema that native creation borrows. Closing only the schema struct leaks the child allocations. Release the export before closing the struct, for both URI and stream writers.The cleanup uses separate try-with-resources entries: the release callback runs before the struct closes, both cleanup steps are attempted, and cleanup failures are suppressed onto an existing export or creation failure. The release callback is checked before invocation so an export that did not install one can still be cleaned up.
Review follow-up
Validation
The original revision was checked with
./gradlew --parallel --continue javadoc check. The latest cleanup and test changes have not been run; build, test, lint, and formatting execution remain the user's choice under repository guidance.I used AI tools to help write and review this change.