[4.0.2 backport] CBG-5034: Add revcache value lock for synchronized delta generation #7911
Conversation
…lta generation
There was a problem hiding this comment.
Pull request overview
This PR backports CBG-5034 to version 4.0.2, adding synchronization for delta generation in the revision cache. The change prevents multiple concurrent clients from generating the same delta simultaneously by introducing a per-revision lock that coordinates delta computation across racing goroutines.
Key Changes:
- Adds a mutex to the revision cache value to synchronize delta generation across concurrent clients
- Refactors the GetDelta flow to check for cached deltas both before and after acquiring the lock
- Includes comprehensive test coverage and benchmarks for concurrent delta sync scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| db/revision_cache_lru.go | Adds deltaLock mutex to revCacheValue struct and passes it to DocumentRevision |
| db/revision_cache_interface.go | Adds RevCacheValueDeltaLock field to DocumentRevision struct |
| db/database_test.go | Adds tests and benchmarks for concurrent delta sync behavior |
| db/crud.go | Refactors GetDelta to use lock-based synchronization and implements double-check pattern for cached deltas |
| } | ||
| var fromRevision DocumentRevision | ||
|
|
||
| var initialFromRevision DocumentRevision |
There was a problem hiding this comment.
[nitpick] The variable name initialFromRevision is unclear about its purpose in the double-check locking pattern. Consider renaming it to cachedFromRevision or unlocked_FromRevision to make it clearer that this is the revision fetched before acquiring the lock.
|
|
||
| isAuthorized, redactedBody := db.authorizeUserForChannels(docID, toRev, fromRevision.CV, fromRevision.Delta.ToChannels, fromRevision.Delta.ToDeleted, encodeRevisions(ctx, docID, fromRevision.Delta.RevisionHistory)) | ||
| // fromRevisionForDiff is a version of the fromRevision that is guarded by the delta lock that we will use to generate the delta (or check again for a newly cached delta) | ||
| var fromRevisionForDiff DocumentRevision |
There was a problem hiding this comment.
[nitpick] The variable name fromRevisionForDiff could be more descriptive. Consider renaming it to lockedFromRevision or fromRevisionAfterLock to indicate this is the revision checked after acquiring the lock in the double-check pattern.
| //{ | ||
| // name: "5MBDoc", | ||
| // docSize: 5 * 1024 * 1024, | ||
| //}, |
There was a problem hiding this comment.
Remove the commented-out test case for '5MBDoc'. If this configuration is intended for future use, consider documenting why it's commented out or remove it entirely to keep the code clean.
| //{ | |
| // name: "5MBDoc", | |
| // docSize: 5 * 1024 * 1024, | |
| //}, |
CBG-5034
Unclean (but trivial) cherry-pick for #7903 to 4.0.2
Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGatewayIntegration/186/