Open max-celus opened 4 months ago
I think work around is to keep a reference 'owned' and then clone it to pass make new connections to pass to threads, as it is done in the r2d2 connection pool manager type. Have you looked at that yet? It will maintain the reference for you.
Ideally, the database and connection structures should be separated as they are done in other client api's into a Database and a Connection type; where you first create a Database, and then you pass an Arc
Thanks for the reply. I already did something similar inside my own code.
I'll await a reply on perhaps changing the API, which I'd be more than happy to do. But if there is no desire to change it then feel free to close the issue :)
After spending some time in a multithreaded program using duckdb I ran into a segmentation fault. I can be a bit more careful in my Rust code, but I was wondering if I could fix it for you. To do so I need some guidance in the way the underlying API works. The failing test can be viewed here. Its code:
The problem is calling
InnerConnection::new(...)
when theffi::duckdb_database
is set to a null pointer after closing.I would like to fix it, but there is some overhead involved. And I need some guidance on the internals of the C-interface to duckdb. We could put the
ffi::duckdb_database
behind some reference counting (like anArc
), and only callffi::duckdb_close(...)
when that database object is dropped. This would introduce an extra pointer indirection through theArc
and the overhead of atomic operations.But to then make the entire implementation a bit more sound we would need to change the calls:
Connection::open_from_raw
: as this would now require anArc
-wrapped object.InnerConnection::close(...)
: must be changed to consumeself
. Which causes a problem inConnection::close(...)
with theConnection::db
, which is aRefCell<InnerConnection>
. As we would have to consume that inner connection as well. To solve this (and to reflect the fact that one cannot share aConnection
across threads anyway, only move it between threads, is to make the methods onConnection
take a&mut self
parameter.Here is where I stopped checking out the code. As I would like to hear if you would even want to see such far-reaching changes to the API.
Perhaps there is another (simpler) solution, some options, with varying degrees of ugliness:
OwnedConnection
that actually owns theInnerConnection
. Only thatOwnedConnection
supportstry_clone
. Can be implemented with typestate asConnection<Owned>
.Err
fromtry_clone
when we detect that the "owned"InnerConnection
has been closed, but this would require some thread-safe sharing of a flag, with the implied overhead.I'd love to hear from you. For now I'll just keep the "owned"
InnerConnection
in a very special place in my code :).