Add Store manager to ensure that we only have one open handle to each database.
This implementation has gone through several iterations. The considerations that formed the current design are as follows:
rusqlite::Connection is Send but not Sync and therefore cannot be stored in a static var. This is why we only store the Conn inside Stores.
Conn is stored as a Weak reference because otherwise every time we return a Store we increase the reference count from one to two, forcing every mutable operation performed on the Conn to clone rather than return a mutable reference, dramatically increasing the number of Conn references proliferating the system. By storing Conn as a Weak, for as long as there is only one Store we can perform mutable operations without increasing the reference count on Conn.
By storing Conn as a Weak reference, if we cease to hold any strong references to the Conn, by not having any active Store's, the Weak will no longer be upgradable, but the entry will remain in the map of open Conns. We therefore must recreate the Conn if a request for a previously opened store is received. We do this in the background without consumers being aware.
The original implementation had a thread_local store of open Rc<rusqlite::Connection>'s mapped by the same key as Conn's. However, due to rusqlite::Connection not being Clone, this meant that Rc::make_mut could not be used, which resulted in the code being unable to get a mutable reference to the rusqlite::Connection if there was more than one reference. As the map retained a strong reference, that ensured that every returned Store had a reference count of at least 2 on it's rusqlite::Connection which resulted in all mutable operations on the Store failing. I therefore made the decision to create a new rusqlite::Connection for every Store returned. This contravenes the original spec, but given the constraints something had to give and I decided on this one.
Writing tests with in memory stores mapped by path ("") with a static Store lead to a problem where the same Conn was shared between tests and the assertions around reference counts on stored Conn's were unreliable. In order to get around this I introduced the concept of the named_in_memory_store which assigns a name to an in memory store ensuring that the same Conn was not shared between tests. This is the requirement that lead to the decision to store keys as String's rather than PathBuf.
The thing I don't like about this is the number of rusqlite::Connections that we create. After chatting with @rnewman I will look into providing a thread_local connection pool and get Stores to take a reference to a connection that it will use to create Storess.
Add Store manager to ensure that we only have one open handle to each database.
This implementation has gone through several iterations. The considerations that formed the current design are as follows:
rusqlite::Connection
isSend
but notSync
and therefore cannot be stored in a static var. This is why we only store theConn
insideStores
.Conn
is stored as aWeak
reference because otherwise every time we return aStore
we increase the reference count from one to two, forcing every mutable operation performed on theConn
to clone rather than return a mutable reference, dramatically increasing the number of Conn references proliferating the system. By storingConn
as aWeak
, for as long as there is only one Store we can perform mutable operations without increasing the reference count onConn
.Conn
as aWeak
reference, if we cease to hold any strong references to theConn
, by not having any activeStore
's, theWeak
will no longer be upgradable, but the entry will remain in the map of openConns
. We therefore must recreate theConn
if a request for a previously opened store is received. We do this in the background without consumers being aware.thread_local
store of openRc<rusqlite::Connection>
's mapped by the same key asConn
's. However, due torusqlite::Connection
not beingClone
, this meant thatRc::make_mut
could not be used, which resulted in the code being unable to get a mutable reference to therusqlite::Connection
if there was more than one reference. As the map retained a strong reference, that ensured that every returned Store had a reference count of at least 2 on it'srusqlite::Connection
which resulted in all mutable operations on theStore
failing. I therefore made the decision to create a newrusqlite::Connection
for every Store returned. This contravenes the original spec, but given the constraints something had to give and I decided on this one.Store
lead to a problem where the sameConn
was shared between tests and the assertions around reference counts on storedConn
's were unreliable. In order to get around this I introduced the concept of thenamed_in_memory_store
which assigns a name to an in memory store ensuring that the sameConn
was not shared between tests. This is the requirement that lead to the decision to store keys asString
's rather thanPathBuf
.The thing I don't like about this is the number of
rusqlite::Connection
s that we create. After chatting with @rnewman I will look into providing athread_local
connection pool and getStores
to take a reference to a connection that it will use to createStores
s.