Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

abci: use a semaphore for commit/apply/beginblock #196

Merged
merged 2 commits into from
Aug 23, 2023
Merged

abci: use a semaphore for commit/apply/beginblock #196

merged 2 commits into from
Aug 23, 2023

Conversation

jchappelow
Copy link
Member

@jchappelow jchappelow commented Aug 17, 2023 •
edited
Loading

This replaces the WaitGroup that was being use to block BeginBlock calls until the asynchronous portion of a previous Commit is applied. This is better than either a mutex or a waitgroup for this use case.

I'm still not certain what is gained by having a portion of the Commit operation being asynchronous however. I can't identify what we are unblocking in CometBFT by having Commit return earlier. AFAICT, the four "connections" that Comet establishes with our application are generally independent. The "local client" does have a single mutex for all for connections, but in general these are independent, and the "connection synchronized" local client and all the remote client options have independent connections for the different contexts. See NewConnSyncLocalClientCreator in cometbft code.

@jchappelow jchappelow force-pushed the commit-gate branch 2 times, most recently from 417efb2 to a1f5814 Compare August 18, 2023 16:55
@jchappelow jchappelow changed the title [concept] abci: try a semaphore for commit/apply/beginblock abci: use a semaphore for commit/apply/beginblock Aug 18, 2023
@jchappelow
Copy link
Member Author

Note for the inevitable cometbft v0.38 upgrade: we'll probably switch to the "connection-synchronized local client" using NewConnSyncLocalClientCreator, which was previous called an "unsynchronized" client:
cometbft/cometbft#1145
A that point, I believe the asynchronous portion of Commit (the wal apply) should cease to be asynchronous (eliminate the callback and have Commit return when it's all actually applided.

@jchappelow jchappelow marked this pull request as ready for review August 22, 2023 00:46
@jchappelow
Copy link
Member Author

Let's try this now that kwild is running again

@jchappelow
abci: use a semaphore for commit/apply/beginblock
083bfdb 
Consensus method requests from cometbft are synchronous, but a portion
of the work of Commit is launched in a goroutine, so we block a
subsequent BeginBlock from starting new changes. We do this by
acquiring a semaphore with max concurrency of 1 at the start of
BeginBlock, and releasing it when the changes from Commit have finished
applying. A mutex is rarely held for longer than the duration of a
local function, while a waitgroup does not provide atomic Wait/Add
semantics that fit here.
brennanjl
brennanjl approved these changes Aug 23, 2023
View reviewed changes
Copy link
Collaborator

@brennanjl brennanjl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@sonarcloud
Copy link

sonarcloud bot commented Aug 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@brennanjl brennanjl merged commit 7b1ec3f into main Aug 23, 2023
2 of 3 checks passed
@brennanjl brennanjl deleted the commit-gate branch August 23, 2023 16:01
@jchappelow jchappelow added this to the v0.6.0 milestone Sep 28, 2023
brennanjl added a commit that referenced this pull request Feb 26, 2024
@jchappelow @brennanjl
abci: use a semaphore for commit/apply/beginblock (#196)
83b9979 
Consensus method requests from cometbft are synchronous, but a portion
of the work of Commit is launched in a goroutine, so we block a
subsequent BeginBlock from starting new changes. We do this by
acquiring a semaphore with max concurrency of 1 at the start of
BeginBlock, and releasing it when the changes from Commit have finished
applying. A mutex is rarely held for longer than the duration of a
local function, while a waitgroup does not provide atomic Wait/Add
semantics that fit here.

Co-authored-by: Brennan Lamey <[email protected]>
brennanjl added a commit that referenced this pull request Feb 26, 2024
@jchappelow @brennanjl
abci: use a semaphore for commit/apply/beginblock (#196)
366bdf8 
Consensus method requests from cometbft are synchronous, but a portion
of the work of Commit is launched in a goroutine, so we block a
subsequent BeginBlock from starting new changes. We do this by
acquiring a semaphore with max concurrency of 1 at the start of
BeginBlock, and releasing it when the changes from Commit have finished
applying. A mutex is rarely held for longer than the duration of a
local function, while a waitgroup does not provide atomic Wait/Add
semantics that fit here.

Co-authored-by: Brennan Lamey <[email protected]>
jchappelow added a commit that referenced this pull request Feb 26, 2024
@jchappelow @brennanjl
abci: use a semaphore for commit/apply/beginblock (#196)
b19b769 
Consensus method requests from cometbft are synchronous, but a portion
of the work of Commit is launched in a goroutine, so we block a
subsequent BeginBlock from starting new changes. We do this by
acquiring a semaphore with max concurrency of 1 at the start of
BeginBlock, and releasing it when the changes from Commit have finished
applying. A mutex is rarely held for longer than the duration of a
local function, while a waitgroup does not provide atomic Wait/Add
semantics that fit here.

Co-authored-by: Brennan Lamey <[email protected]>
brennanjl added a commit that referenced this pull request Feb 26, 2024
@jchappelow @brennanjl
abci: use a semaphore for commit/apply/beginblock (#196)
3f28ca6 
Consensus method requests from cometbft are synchronous, but a portion
of the work of Commit is launched in a goroutine, so we block a
subsequent BeginBlock from starting new changes. We do this by
acquiring a semaphore with max concurrency of 1 at the start of
BeginBlock, and releasing it when the changes from Commit have finished
applying. A mutex is rarely held for longer than the duration of a
local function, while a waitgroup does not provide atomic Wait/Add
semantics that fit here.

Co-authored-by: Brennan Lamey <[email protected]>
brennanjl added a commit that referenced this pull request Feb 26, 2024
@jchappelow @brennanjl
abci: use a semaphore for commit/apply/beginblock (#196)
8c458ce 
Consensus method requests from cometbft are synchronous, but a portion
of the work of Commit is launched in a goroutine, so we block a
subsequent BeginBlock from starting new changes. We do this by
acquiring a semaphore with max concurrency of 1 at the start of
BeginBlock, and releasing it when the changes from Commit have finished
applying. A mutex is rarely held for longer than the duration of a
local function, while a waitgroup does not provide atomic Wait/Add
semantics that fit here.

Co-authored-by: Brennan Lamey <[email protected]>
brennanjl added a commit that referenced this pull request Feb 26, 2024
@jchappelow @brennanjl
abci: use a semaphore for commit/apply/beginblock (#196)
490e9a5 
Consensus method requests from cometbft are synchronous, but a portion
of the work of Commit is launched in a goroutine, so we block a
subsequent BeginBlock from starting new changes. We do this by
acquiring a semaphore with max concurrency of 1 at the start of
BeginBlock, and releasing it when the changes from Commit have finished
applying. A mutex is rarely held for longer than the duration of a
local function, while a waitgroup does not provide atomic Wait/Add
semantics that fit here.

Co-authored-by: Brennan Lamey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brennanjl brennanjl brennanjl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@jchappelow @brennanjl