fix: Handle concurrent MemDB writes - #3239
Conversation
github-actions
Bot
added
area/datastore
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Requires review @miparnisari |
| } | ||
|
|
||
| // TestConcurrentWriteRevisionInversionLosesRead reproduces concurrent writes to MemDB | ||
| func TestConcurrentWriteRevisionInversionLosesRead(t *testing.T) { |
There was a problem hiding this comment.
does this test reproduce the error 100% of the time? if not, can we make it so? or at least make it so that it fails a large percentage of the runs?
e.g. run a for loop 1000 times where in each iteration we spin up two concurrent goroutines?
There was a problem hiding this comment.
Yes, its deterministic. The test fails 100% of the time if the code fix introduced is removed.
The test manually simluates goroutine B which has the latest revision number to complete its execution before goroutine A which has the earlier revision number.
This is done using channels and its the same reason why wait groups are not used as they will not ensure the order of execution that goroutine B completes the execution before A is unblocked and proceeds to commit..
Added docstrings mentioning the test scenario
There was a problem hiding this comment.
Your test didn't have 100% pass rate when ran many times (there was another bug - the collision guard in newRevisionID).
I've updated it to account for this, plus i've added more assertions.
| schemaHash := mdb.getCurrentSchemaHashNoLock() | ||
| snap := mdb.db.Snapshot() | ||
| mdb.revisions = append(mdb.revisions, snapshot{newRevision, schemaHash, snap}) | ||
| mdb.insertRevisionSnapshot(newRevision, schemaHash, snap) |
There was a problem hiding this comment.
i'm confused... you said
Root cause: a transaction's revision number is stamped before it acquires the write lock
isn't the solution to acquire the lock? and which lock are you talking about? i see we're acquiring on line 249 already.
There was a problem hiding this comment.
Yes the lock mdb.Lock() is already existing as you mentioned, but this is released and acquired thrice in ReadWriteTx by a single goroutine/transaction.
In between the release and acquire actions, another transaction B is able to run its own ReadWriteTx to completion, including its commit and snapshot recording before A resumes.
A single global lock can be done per transaction for ReadWriteTx, but this would affect the concurrency and serialize all the transaction in this operation.
Instead we can ensure that the snapshots are inserted in the sorted order as per their revision ID regardless of the transaction's commit order. This is the fix that is implemented in this PR.
There is a little gap in the PR description, will update it mentioning this.
There was a problem hiding this comment.
Your fix addresses the symptom but not the root cause. I've updated the code to fix the root cause.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…nt-memdb-writes' into fix/handle-concurrent-memdb-writes
ff4aa4a to
d123966
Compare
d123966 to
694389e
Compare
|
@miparnisari |
github-actions
Bot
removed
the
area/tooling
|
@miparnisari |
|
Since I made changes to your PR, i'm waiting for someone else in the maintainer team to review. @tstirrat15 can you review? |
2 participants
Description
Fixes #3212: concurrent writes to the
memdbdatastore could acknowledge aWriteRelationshipscall whose data was then invisible to an immediately following fully-consistent read.Root cause: within a single
ReadWriteTx,mdb.Lock()is acquired and released in three separate windows (assigning the revision number, claiming the write-transaction slot, and committing + recording the snapshot) rather than held for the whole transaction.Between those windows, another transaction can fully commit, so two transactions can be recorded in a different order than their revision numbers were assigned in. Snapshots were recorded via a plain
append, somdb.revisionscould end up out of order, andSnapshotReader's binary search assumes it's sorted, so it could resolve to a stale snapshot that predates the write being requested.Fix: Holding one lock for a transaction's entire duration would also fix the ordering, but at the cost of serializing all transactions.
Instead,
insertRevisionSnapshotinserts each snapshot at its correct sorted position based on revision ID, regardless of the order transactions actually commit in.Testing
Added
TestConcurrentWriteRevisionInversionLosesRead, which forces the scenario that triggers the bug and asserts a read at a transaction's own revision sees its write.Test result without adding the fix for insertion:
References