Introduce a store transaction for Account
#2660
Conversation
…a` and identify tests
…of `Store::save_account` + `Account::save`
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2660 +/- ##
==========================================
+ Coverage 78.21% 78.25% +0.03%
==========================================
Files 191 191
Lines 19902 19933 +31
==========================================
+ Hits 15567 15598 +31
Misses 4335 4335
☔ View full report in Codecov by Sentry. |
| @@ -138,6 +138,11 @@ impl CryptoStore for MemoryStore { | |||
| Ok(self.next_batch_token.read().await.clone()) | |||
| } | |||
|
|
|||
| async fn save_pending_changes(&self, _changes: PendingChanges) -> Result<()> { | |||
| // TODO(bnjbvr) why didn't save_changes save the account? | |||
There was a problem hiding this comment.
The account is, or perhaps was, only loaded when you create the OlmMachine. Once that's done the Account lives and dies with the OlmMachine.
This made it unnecessary for the MemoryStore to persist the account.
There was a problem hiding this comment.
Interesting. With this new design, the ReadOnlyAccount would only live transiently:
- either read from the database, available from the
cache() - or we plan to mutably use it, which implies the use of a
transaction()
In that case, we would need to save it for the memory store.
(Note that, at the end of the day, the memory store could be exactly the same thing as the in-memory cache 🤪 and since a cross-process lock for the memory store doesn't make sense, all the methods in the memory stub could do strictly nothing, since the cache would keep the values in memory 🧠)
| // Note: bnjbvr lost against borrowck here. Ideally, the `F` parameter would take a | ||
| // `&StoreTransaction`, but callers didn't quite like that. | ||
| #[cfg(test)] | ||
| pub(crate) async fn with_transaction< |
There was a problem hiding this comment.
Hmm, that's more what I had in mind, why is this for tests only? Can't we mimic the sqlx interface here? They seem to have figured out how to make the borrow checker happy: https://docs.rs/sqlx/latest/sqlx/prelude/trait.Connection.html#method.transaction.
There was a problem hiding this comment.
We can definitely use it outside tests too; I found it nice to use in tests because it creates a small scope, but in general code where the transaction may live longer, it was good enough to have an explicit commit. Both are fine to me, I don't have a strong opinion here.
whenever we can use `StaticAccountData` in place.
bnjbvr
changed the title
ReadOnlyAccountAccount
| self.store.save_changes(changes).await?; | ||
| } | ||
| SessionType::Existing(s) => { | ||
| SessionType::New(s) | SessionType::Existing(s) => { |
There was a problem hiding this comment.
Hmm, a little bit uneasy about this one. We're now persisting the Session but not the Account. This means that the Account will contain the one-time key to create the session for a bit longer than needed.
I guess there's nothing we can do here until we move the Session to use the transaction as well.
This adds a new
StoreTransactiontype, that wraps aStoreCacheand aStore. The idea is that it will allow write access to theStore(and maintains the cache at the same time), while theStore::cachemethod will only give read-only access to the store's content.Another new data type is introduced,
PendingChanges, that reflectsChangesbut for fields that are properly maintained in theStoreCacheand that one can write in theStoreTransaction. In the future, it wouldn't be possible to usesave_pending_changesfrom the outside of aStoreTransactioncontext.The layering is the following:
Storewraps theDynCryptoStore, contains a reference to aStoreCache.Store::cache().StoreTransaction(later, only one at a time will be allowed, by putting theStoreCachebehind aRwLock; this has been deferred to not make the PR grow too much).StoreCachewill get a method to get a reference to the cached thing: it will either load from the DB if not cached, or return the previously cached value.StoreTransaction: it will either load from the cache into aPendingChangesscratch pad, or return the scratchpad temporary value.StoreTransaction::commit()happens, fields are backpropagated into the DB and the cache.Then, this
StoreTransactionis used to update aReadOnlyAccountin multiple places (and usage ofReadOnlyAccountis minimized so as not to require a transaction or cache function call as much as possible). With this, the read-only account only exists transiently, and it's only stored long-term in the cache.Followup PRs include:
ReadOnlyAccountnot cloneableReadOnlyAccountRwLockon theStoreTransactionPart of #2624 + #2000.