refactor: replace PersistSemaphore with PersistChannel#1710
Merged
RodrigoVillar merged 9 commits intomainfrom Mar 5, 2026
Merged
refactor: replace PersistSemaphore with PersistChannel#1710RodrigoVillar merged 9 commits intomainfrom
PersistSemaphore with PersistChannel#1710RodrigoVillar merged 9 commits intomainfrom
Conversation
PersistSemaphore with locks and condvars
a0d2f34 to
d63cb45
Compare
This was
linked to
issues
Feb 25, 2026
9757aa7 to
0a22f43
Compare
PersistSemaphore with locks and condvarsPersistSemaphore with PersistChannel
demosdemon
reviewed
Feb 26, 2026
bernard-avalabs
requested changes
Mar 2, 2026
Contributor
bernard-avalabs
left a comment
There was a problem hiding this comment.
A few minor suggestions.
a1dab9d to
b72ad76
Compare
bernard-avalabs
approved these changes
Mar 3, 2026
b72ad76 to
4c33dee
Compare
4c33dee to
8154a8c
Compare
rkuris
approved these changes
Mar 4, 2026
Member
rkuris
left a comment
There was a problem hiding this comment.
Mostly nits and some questions. Please re-request review if you make substantial changes.
9590622 to
fef1e1f
Compare
demosdemon
approved these changes
Mar 5, 2026
Contributor
demosdemon
left a comment
There was a problem hiding this comment.
just nits on overflowing arithmetic
fef1e1f to
f7d32d7
Compare
This version might be slightly more efficient and has tighter backpressure at the cost of a higher synchronization complexity. IMO the channel version is more flexible and straightforward for message ordering and multiple work types. It does have more moving parts and ends out creating a bit more work for the worker thread.
adding docs
f7d32d7 to
2242401
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this should be merged
Quoting #1694:
How this works
Replaces the
PersistSemaphore+crossbeam::channelwithPersistChannel, a channel-like abstraction around the locks/condvars mentioned in #1694. By usingPersistChannel, we no longer have the issue of thePersistWorkerand thePersistLoophaving their own notions of progress as all progress is managed viaPersistChannelState.This PR also adds a drop guard to the
PersistLoop, such that if the background thread exits for whatever reason, the system is marked as shutdown which prevents thePersistWorkerfrom hanging.How this was tested
CI + existing deferred persistence UTs