GH-1205: Fix byte-array element leak in FromSchemaByteArray - #1249
GH-1205: Fix byte-array element leak in FromSchemaByteArray#1249PG1204 wants to merge 2 commits into
Conversation
…releasing elements via RAII guard
This comment has been minimized.
This comment has been minimized.
|
Seems reasonable. From what I was researching, doesn't seem like it is a leak easy to hit and it may only leak a few KBs. I'd probably just try to add a test that reproduces the issue an ensure that it passes with the fix |
@xborder Agreed it's low-impact and hard to hit. I've added a regression test (TestFromSchemaByteArray) that feeds malformed schema bytes to the native createDataset path, which is the exact branch that used to skip ReleaseByteArrayElements, and confirms it now fails gracefully with the fix in place. |
|
@PG1204 I don't think this is testing that we fixed the memory leak. This is just testing the error is thrown. Shouldn't it check no memory was leaked? |
@xborder you're right, that only proves the error path is hit, not that the leak is gone. The catch is what leaks: the buffer wraps the pointer from GetByteArrayElements, which on HotSpot copies the array into a JVM-internal malloc'd buffer, and that copy is what leaked. It's not tracked by NativeMemoryPool, not on the Java heap, and has no portable Java API; it's only visible via process RSS. So the only way I see to assert "no leak" from Java is an RSS-growth check: loop the call many times with a large payload and assert RSS stays bounded (unfixed balloons by GBs, fixed stays flat). That works on Linux but needs gating on macOS/Windows and is a bit flaky. Would you prefer (1) that RSS-based test, or (2) dropping the Java assertion and verifying manually with a local leak-checker (valgrind/ASAN), keeping the current test as a graceful-failure guard? Note CI doesn't run a sanitizer today, so option 2 wouldn't be enforced automatically. Happy either way, just want to avoid landing a flaky test. |
What's Changed
FromSchemaByteArray()indataset/src/main/cpp/jni_util.ccacquires the Java byte-array elements viaGetByteArrayElements(), but the matchingReleaseByteArrayElements()was only reached on the success path. Whenarrow::ipc::ReadSchema()failed,ARROW_ASSIGN_OR_RAISEreturned early and skipped the release, leaking the pinned/copied array buffer on every call with malformed or incompatible serialized schema bytes (e.g. viacreateDataset()).This releases the elements through an RAII guard so they are always freed on every exit path - success or error, and stays correct if additional early returns are added later.
Also adds a regression test (
TestFromSchemaByteArray) that repeatedly drives the nativecreateDatasetpath with malformed schema bytes, exercising the former leak branch and asserting it fails gracefully with an exception rather than crashing.Closes #1205.