Copilot commented on code in PR #4441: URL: https://github.com/apache/arrow-adbc/pull/4441#discussion_r3464351207
########## java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/OwnedReferences.java: ########## @@ -0,0 +1,57 @@ +/* + * 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.adbc.driver.jni.impl; + +import java.lang.ref.WeakReference; +import java.util.Collections; +import java.util.Objects; +import java.util.Set; +import java.util.WeakHashMap; +import java.util.stream.Collectors; +import org.apache.arrow.util.AutoCloseables; + +/** + * Track child resources for the ADBC FFI. + * + * <p>You are supposed to close statements before closing the connection (etc.). This class helps + * track those references to prevent misuse at runtime. + */ +public final class OwnedReferences implements AutoCloseable { + private final Set<WeakReference<AutoCloseable>> openReferences; + + public OwnedReferences() { + this.openReferences = Collections.newSetFromMap(new WeakHashMap<>()); + } Review Comment: Using `Set<WeakReference<AutoCloseable>>` backed by a `WeakHashMap` breaks tracking: the `WeakReference` keys themselves are only weakly referenced by the map, so entries can disappear immediately, and child resources won't be reliably closed when the parent closes. ########## java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/OwnedReferences.java: ########## @@ -0,0 +1,57 @@ +/* + * 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.adbc.driver.jni.impl; + +import java.lang.ref.WeakReference; +import java.util.Collections; +import java.util.Objects; +import java.util.Set; +import java.util.WeakHashMap; +import java.util.stream.Collectors; +import org.apache.arrow.util.AutoCloseables; + +/** + * Track child resources for the ADBC FFI. + * + * <p>You are supposed to close statements before closing the connection (etc.). This class helps + * track those references to prevent misuse at runtime. + */ +public final class OwnedReferences implements AutoCloseable { + private final Set<WeakReference<AutoCloseable>> openReferences; + + public OwnedReferences() { + this.openReferences = Collections.newSetFromMap(new WeakHashMap<>()); + } + + public void close() throws Exception { + AutoCloseables.close( + openReferences.stream() + .map(WeakReference::get) + .filter(Objects::nonNull) + .collect(Collectors.toList())); + openReferences.clear(); + } + + public void addReference(AutoCloseable any) { + openReferences.add(new WeakReference<>(any)); + } + + public void releaseReference(AutoCloseable any) { + openReferences.remove(new WeakReference<>(any)); + } Review Comment: `releaseReference()` creates a new `WeakReference` and tries to remove it from the set; that will never match the previously-added `WeakReference` instance, so statements will remain tracked forever (and may be closed multiple times). Track the `AutoCloseable` directly so add/remove operate on the same key. ########## java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/OwnedReferences.java: ########## @@ -0,0 +1,57 @@ +/* + * 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.adbc.driver.jni.impl; + +import java.lang.ref.WeakReference; +import java.util.Collections; +import java.util.Objects; +import java.util.Set; +import java.util.WeakHashMap; +import java.util.stream.Collectors; +import org.apache.arrow.util.AutoCloseables; + +/** + * Track child resources for the ADBC FFI. + * + * <p>You are supposed to close statements before closing the connection (etc.). This class helps + * track those references to prevent misuse at runtime. + */ +public final class OwnedReferences implements AutoCloseable { + private final Set<WeakReference<AutoCloseable>> openReferences; + + public OwnedReferences() { + this.openReferences = Collections.newSetFromMap(new WeakHashMap<>()); + } + + public void close() throws Exception { + AutoCloseables.close( + openReferences.stream() + .map(WeakReference::get) + .filter(Objects::nonNull) + .collect(Collectors.toList())); + openReferences.clear(); + } Review Comment: `close()` currently closes only objects still reachable via `WeakReference.get()`, so it can silently skip still-open statements if the weak references were cleared. Also, clearing the set should happen even if closing one child throws, and it’s safer to close a snapshot since `AutoCloseable.close()` may call back into `releaseReference()` and mutate the set. ########## java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/TiedArrowReader.java: ########## @@ -0,0 +1,96 @@ +/* + * 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.adbc.driver.jni.impl; + +import java.io.IOException; +import java.util.Map; +import java.util.Set; +import org.apache.arrow.memory.BufferAllocator; +import org.apache.arrow.vector.VectorSchemaRoot; +import org.apache.arrow.vector.dictionary.Dictionary; +import org.apache.arrow.vector.ipc.ArrowReader; +import org.apache.arrow.vector.types.pojo.Schema; + +/** A proxy {@link ArrowReader} that keeps an associated ADBC resource alive. */ +class TiedArrowReader extends ArrowReader { + private final ArrowReader delegate; + + @SuppressWarnings("unused") + private AutoCloseable parentHandle; + + TiedArrowReader(BufferAllocator allocator, ArrowReader delegate, AutoCloseable parentHandle) { + // XXX: ArrowReader being an abstract class and not an interface is a massive wart in arrow-java + // design + super(allocator); + this.delegate = delegate; + this.parentHandle = parentHandle; + } + + @Override + public void close(boolean closeReadSource) throws IOException { + delegate.close(closeReadSource); + parentHandle = null; // do NOT close + } + + @Override + public void close() throws IOException { + delegate.close(); + parentHandle = null; // do NOT close + } Review Comment: Same as above for `close()`: `parentHandle` should be cleared even when `delegate.close()` throws, to avoid retaining the parent resource longer than necessary. ########## java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/OwnedReferences.java: ########## @@ -0,0 +1,57 @@ +/* + * 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.adbc.driver.jni.impl; + +import java.lang.ref.WeakReference; +import java.util.Collections; +import java.util.Objects; +import java.util.Set; +import java.util.WeakHashMap; +import java.util.stream.Collectors; +import org.apache.arrow.util.AutoCloseables; Review Comment: `OwnedReferences` no longer needs `WeakReference`/`WeakHashMap` imports once it tracks child `AutoCloseable`s directly; leaving them will trigger unused-import compilation failures. The revised `close()` implementation also needs `ArrayList` for snapshotting before closing. ########## java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/TiedArrowReader.java: ########## @@ -0,0 +1,96 @@ +/* + * 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.adbc.driver.jni.impl; + +import java.io.IOException; +import java.util.Map; +import java.util.Set; +import org.apache.arrow.memory.BufferAllocator; +import org.apache.arrow.vector.VectorSchemaRoot; +import org.apache.arrow.vector.dictionary.Dictionary; +import org.apache.arrow.vector.ipc.ArrowReader; +import org.apache.arrow.vector.types.pojo.Schema; + +/** A proxy {@link ArrowReader} that keeps an associated ADBC resource alive. */ +class TiedArrowReader extends ArrowReader { + private final ArrowReader delegate; + + @SuppressWarnings("unused") + private AutoCloseable parentHandle; + + TiedArrowReader(BufferAllocator allocator, ArrowReader delegate, AutoCloseable parentHandle) { + // XXX: ArrowReader being an abstract class and not an interface is a massive wart in arrow-java + // design + super(allocator); + this.delegate = delegate; + this.parentHandle = parentHandle; + } + + @Override + public void close(boolean closeReadSource) throws IOException { + delegate.close(closeReadSource); + parentHandle = null; // do NOT close + } Review Comment: If `delegate.close(...)` throws, `parentHandle` is never nulled out, which can unnecessarily retain the parent resource after a failed close. Clearing it in a `finally` avoids that retention without changing behavior otherwise. -- 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]
