feat(java/driver/jni): close child resources automatically#4441
Conversation
There was a problem hiding this comment.
Pull request overview
This PR experiments with enforcing parent/child ADBC resource lifetimes in the Java JNI driver (similar to the Python driver manager), so that closing a connection also closes any statements/readers derived from it and prevents use-after-close.
Changes:
- Add
OwnedReferencesto track and close child resources when the parent connection closes. - Tie
ArrowReaderlifetimes to the producing connection/statement viaTiedArrowReaderand updatedimportStream(...)plumbing. - Add a regression test ensuring statements become unusable after their connection is closed.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| java/driver/jni/src/test/java/org/apache/arrow/adbc/driver/jni/JniDriverTest.java | Adds a test asserting connection close invalidates/cleans up statements. |
| java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/JniStatement.java | Registers statements as owned by the parent connection and ties query readers to the statement lifetime. |
| java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/JniConnection.java | Tracks statements/readers created from the connection and closes them on connection close. |
| java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/TiedArrowReader.java | New delegating ArrowReader that retains a parent handle to keep it reachable while reading. |
| java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/OwnedReferences.java | New resource-tracking helper for closing child resources. |
| java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/NativeQueryResult.java | Updates stream import to return a TiedArrowReader associated with the producing resource. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lidavidm
changed the title
amoeba
left a comment
amoeba
left a comment
There was a problem hiding this comment.
Looks good, just one question.
| if (parent != null) { | ||
| parent.getChildReferences().releaseReference(this); | ||
| this.parent = null; | ||
| } |
There was a problem hiding this comment.
Should this and other similar blocks be in a finally so the child always gets detached? Just curious.
| public ChildReferences() { | ||
| this.openReferences = Collections.newSetFromMap(new WeakHashMap<>()); | ||
| } |
There was a problem hiding this comment.
Should we add a one-line comment just to point out that WeakHashMap isn't thread-safe so that we don't overlook that in any future changes?
| public void close() throws Exception { | ||
| AutoCloseables.close(childReferences, handle); | ||
| } |
There was a problem hiding this comment.
JniConnection.close() and JniStatement.close() wrap failures with AdbcException.internal(...) but here we're just declaring throws exception and letting it propagate. Any reason for the difference? Should we wrap here also for consistency?
| refs.close(); | ||
| } | ||
|
|
||
| static final class Closeable implements AutoCloseable { |
There was a problem hiding this comment.
this shadows the java.io.Closeable import which ends up being unused I think. Should we drop the import?
This mimics what the Python driver manager does. Unlike Python, it simply closes the children. It does not protect against concurrent usage.