Change the store traits so they expose a transactional API

[bnjbvr]

This would increase robustness in the case of the two sync loops, so that we're sure that there aren't any partial write to the db that the main loop consumer would try to read later.

[poljar]

To explain a bit further, since we do use transactions for all of our writes.

Currently all the changes we want to write are first collected into a struct named Changes. Then we call Store.save_changes() which saves everything we collected in a single transaction.

This was mostly done since sled has an API where you pass in a closure which gets executed as part of a transaction. In this closure you write your values to the db and everybody is happy.

But most other databases have a Database::get_transaction(&self) -> &Transaction interface. Crucially SQLite and IndexedDB behave this way.

We could change our store traits to reflect this new reality and get rid of the Store::save_changes(&self, changes: Changes) API and add a Store::get_transaction(&self) API instead.

This way, if we need to do reads during a write, they also become part of the transaction.

[bnjbvr]

Wait, turns out that sqlite implementations for both the crypto store trait and the state store trait make use of sqlite transactions. So it's a non issue, as it's already implemented, or are you suggesting something else?

[poljar]

I am.

If you take a look at the CryptoStore trait:

matrix-rust-sdk/crates/matrix-sdk-crypto/src/store/traits.rs

Line 59 in d6d19d9

async fn save_changes(&self, changes: Changes) -> Result<(), Self::Error>;

We have this method, which may or may not open a transaction behind the scenes, our current implementations do so.

But we also have some other methods:

matrix-rust-sdk/crates/matrix-sdk-crypto/src/store/traits.rs

Lines 219 to 228 in d6d19d9

	async fn get_custom_value(&self, key: &str) -> Result<Option<Vec<u8>>, Self::Error>;
	/// Put arbitrary data into the store
	///
	/// # Arguments
	///
	/// * `key` - The key to insert data into
	///
	/// * `value` - The value to insert
	async fn set_custom_value(&self, key: &str, value: Vec<u8>) -> Result<(), Self::Error>;

Currently we can't store two custom values in a single transaction. Any reads that may happen before a save_changes() call also aren't part of the transaction which the save_changes() method will use.

If we instead have a method in the CryptoStore trait which returns a transaction, and then the transaction object exposes a set_custom_value() method, then we could store multiple values in a single transaction.

That being said, none of this is required currently, and the faux transaction using save_changes() will work just fine.

The above change to our store traits has been previously suggested by @jplatte, so he may have even more concrete ideas.

[richvdh]

This was mostly done since sled has an API where you pass in a closure which gets executed as part of a transaction. In this closure you write your values to the db and everybody is happy.

But most other databases have a Database::get_transaction(&self) -> &Transaction interface. Crucially SQLite and IndexedDB behave this way.

You have to be careful with IndexedDB transactions: in particular you can't keep them open indefinitely; they will terminate as soon as you stop giving them work to do, so you can't do any async operations. That may be fine here, but it's a footgun that needs bearing in mind.

(Indeed, IndexedDB's actual API also takes closures that you pass into IndexedDB to be executed as part of the transaction. rust-indexed-db just puts an abstraction layer over it to make it look like a regular async thing, but the abstraction is inherently leaky.)

[richvdh]

We no longer believe this approach is correct.