felipecrv commented on PR #2713: URL: https://github.com/apache/arrow-adbc/pull/2713#issuecomment-2816186776
> > If you don't want to trust the ADBC driver implementations to handle the reference counting and weak references to connection (I would) you can keep an inner Arc inside every Connection and avoid trying to track this complex relationship statically. > > Is what you are describing provided by the driver manager? > > https://github.com/apache/arrow-adbc/blob/7f1dfca741c298209e213d00d785c7a220bdc0bb/rust/core/src/driver_manager.rs#L38-L41 What I'm suggesting would require a `ManagedRecordBatchReader` that wraps a weak-pointer to the managed connection and the underlying record batch reader that comes from the FFI. So `ManagedRecordBatchReader::Next()` tries to upgrade the weak reference to a strong one, then it calls next on the underlying reader. If the upgrade fails, `Next()` immediately returns an error. This would make readers (1) safe when the underlying reader in the driver doesn't take care of that, and (2) dangling readers wouldn't prevent connection destruction since the readers hold a weak reference to the connection. Another good consequence of (2) is that connection destruction is not blocked by all the record batch readers that originated from that connection. An alternative to that is trusting that the driver itself takes care of that (they should) and inheriting the safety (or lack of) properties of the driver when it comes to managing the relationship between readers and connections. a) track relationship with static types (too restrictive and high cognitive overload) b) tracking relationship dynamically with weak references (safe, flexible, easy to use, but adds another layer of ref-counting) c) trusting that the driver's `RecordBatchReader::Next()` will fail when the shared connection resources are not available anymore (as safe as the driver is, easy to use, flexible, no extra layer of ref-counting) Link to docs on `Weak` refs: https://doc.rust-lang.org/std/sync/struct.Weak.html#method.upgrade (I believe what we have here is a textbook application of weak references. It's to deal with these problems that they exist). -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
