Closes #660 Store management #260

New issue

Open

opened 2020-08-06 16:57:30 +00:00 by gburd · 0 comments

gburd commented

2020-08-06 16:57:30 +00:00

(Migrated from github.com)

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: 1. `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`. 2. `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`. 3. 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. 4. 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](#660), but given the constraints something had to give and I decided on this one. 5. 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::Connection`s 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 `Stores`s.