Global tx for local and sync#3428
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on February 27. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Takeno
force-pushed
the
feat/global-tx
branch
from
49c6eba to
ed21ab3
Compare
nrainhart
left a comment
nrainhart
left a comment
There was a problem hiding this comment.
The new API looks good! I feel like the final implementation should provide stronger guarantees, such as:
- roll back in-memory changes if an error is thrown during the callback execution
- store batch transactionally on the server
- (optionally) waiting for confirmation from the server before resolving the promise?
But we can start trying out this version now.
As a sidenote, I wonder if using withTransaction actually introduces any noticeable performance improvements when doing several operations at the same time. Would it make sense to use it in methods like applyDiff and CoList.push?
| export class SQLiteTransactionAsync implements DBTransactionInterfaceAsync { | ||
| constructor(private readonly tx: SQLiteDatabaseDriverAsync) {} | ||
|
|
||
| async upsertCoValue( |
There was a problem hiding this comment.
I don't love that we're duplicating upsertCoValue in the DB client and transaction APIs. I think we need to rethink this DB client/transaction split, because in many cases we want to run queries both inside and outside a transaction, and this leads us to either code duplication or creating unnecessary transactions (see also my previous comment) cc @gdorsi
We don't need to fix this in this PR, thought. Maybe let's just add a comment to keep both implementations from drifting apart
Takeno
force-pushed
the
feat/global-tx
branch
from
085610a to
acabf4c
Compare
I observed some performance improvements in browser-perf-tests, creating 50x50 grids, from ~2300ms to ~1900ms. |
nrainhart
left a comment
nrainhart
left a comment
There was a problem hiding this comment.
Leaving a few more comments, but only one's important.
Should we rename withTransaction to something more like batchUpdates
Yeah, this actually may be better to avoid setting incorrect expectations. Wanna discuss this with the rest of the team?
I would start releasing this under unstable_ prefix, gather feedback, and later opt-in in some internals like applyDiff / push.
Sounds good!
| this.trySendToPeer(peer, content); | ||
| peer.combineOptimisticWith(coValue.id, contentKnownState); | ||
| peer.trackToldKnownState(coValue.id); | ||
| for (const content of contents) { |
There was a problem hiding this comment.
It's a bit confusing that the two content variables here refer to different things. We could rename the param to contentMsg
| : Promise.resolve(undefined); | ||
|
|
||
| const contentKnownState = knownStateFromContent(content); | ||
| const syncPromise = this.syncContent({ |
There was a problem hiding this comment.
There was a problem hiding this comment.
Is there any reason not to have the same test for StorageApiAsync?
| return new Promise((resolve, reject) => { | ||
| this.dbClient.transaction((tx) => { | ||
| return callback(tx).then(resolve, reject); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Can we always pass a transaction instead? That way for regular stores we use one tx per store operation
Co-authored-by: Nico Rainhart <nmrainhart@gmail.com>
2 participants
This PR adds atomic transactions for CoValue mutations: multiple mutations can be executed inside a single
withTransaction()callback and are then persisted together in one IndexedDB transaction and synced together in one networkBatchMessage. Mutations are applied to memory immediately (optimistic); persistence and sync happen once the callback returns, with a single attempt for each. No automatic retry and no rollback of in-memory state.This is primarily to protect against the client being closed in the middle of a sync/store operation.
It is based on already existing concepts of
DBTransactionInterfaceAsync.DX
Critical changes
upsertCoValuebased on an existing transaction.Limitations
withTransactioninteract with coValues mutated outside, the indexeddb tx might fail becauseupsertCoValuelooks inside the database and may not find the CoValue due to transaction concurrency.Batchmessage's payload sent to the sync server might be bigger than transport limits for a single message.NewContentmessage is received.