threading: drop CSemaphore in favor of c++20 std::counting

theuni commented at 1:00 am on May 10, 2025: member

This is relatively simple, but done in a bunch of commits to enable scripted diffs.

I wanted to add a semaphore in a branch I’ve been working on, but it was unclear if I should use std::counting_semaphore or stick with our old CSemaphore. I couldn’t decide, so I just decided to remove all doubt and get rid of ours :)

This replaces our old CSemaphore with std::counting_semaphore everywhere we used it. CSemaphoreGrant is still there as an RAII wrapper, but is now called CountingSemaphoreGrant and BinarySemaphoreGrant to match. Those have been moved out of sync.h to their own file.

threading: rename CSemaphore methods to match std::semaphore 7b816c4e00

threading: add temporary semaphore aliases d870bc9451

scripted-diff: rename CSemaphoreGrant and CSemaphore for net

-BEGIN VERIFY SCRIPT-
sed -i -e 's|CSemaphoreGrant|SemaphoreGrant|g' -e 's|CSemaphore|Semaphore|g' src/net.h src/net.cpp
-END VERIFY SCRIPT-

6790ad27f1

wallet: change the write semaphore to a BinarySemaphore

Follow-up commits will make a distinction between Semaphore and
BinarySemaphore.

793166d381

scripted-diff: rename CSemaphore and CSemaphoreGrant

CountingSemaphore and CountingSemaphoreGrant model std::counting_semaphore.

-BEGIN VERIFY SCRIPT-
sed -i -e 's|CSemaphoreGrant|CountingSemaphoreGrant|g' -e 's|CSemaphore|CountingSemaphore|g' src/sync.h
-END VERIFY SCRIPT-

e6ce5f9e78

threading: make CountingSemaphore/CountingSemaphoreGrant template types 1acacfbad7

threading: replace CountingSemaphore with std::counting_semaphore f21365c4fc

scripted-diff: threading: semaphore: use direct types rather than the temporary convenience ones

-BEGIN VERIFY SCRIPT-
sed -i 's|BinarySemaphore|std::binary_semaphore|g' src/wallet/sqlite.h
sed -i 's|SemaphoreGrant|CountingGrant|g' src/net.h src/net.cpp
sed -i 's|Semaphore|std::counting_semaphore<>|g' src/net.h src/net.cpp
sed -i 's|CountingGrant|CountingSemaphoreGrant<>|g' src/net.h src/net.cpp

-END VERIFY SCRIPT-

1f89e2a49a

threading: semaphore: remove temporary convenience types fd15469892

DrahtBot commented at 1:00 am on May 10, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32466.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	purpleKarrot, TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32394 (net: make m_nodes_mutex non-recursive by vasild)
#32065 (i2p: make a time gap between creating transient sessions and using them by vasild)
#32015 (net: replace manual reference counting of CNode with shared_ptr by vasild)
#30988 (Split CConnman by vasild)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#28584 (Fuzz: extend CConnman tests by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label CI failed on May 10, 2025

DrahtBot commented at 2:45 am on May 10, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/41976150929 LLM reason (✨ experimental): The CI failure is caused by a linting error related to a missing include guard in src/semaphore_grant.h.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

threading: semaphore: move CountingSemaphoreGrant to its own header 6f7052a7b9

theuni force-pushed on May 10, 2025

DrahtBot removed the label CI failed on May 10, 2025

laanwj added the label Utils/log/libs on May 10, 2025

shahsb commented at 7:36 am on May 11, 2025: none

LGTM.. Thanks for making the changes!

purpleKarrot commented at 10:08 am on May 11, 2025: contributor

When looking at the new file alone, I see several oportunities for improvement: Drop the f prefix, use default member initializers, use std::exchange. But when looking at the individual commits, I see that this is just moving existing code around, so it makes sense to do defer additional cleanup to a separate PR.

The code would have been easier to review if the last commit was not part of this PR, but it looks good.

ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68

TheCharlatan approved

TheCharlatan commented at 10:40 am on May 12, 2025: contributor

ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68

theuni commented at 3:14 pm on May 15, 2025: member

When looking at the new file alone, I see several oportunities for improvement: Drop the f prefix, use default member initializers, use std::exchange. But when looking at the individual commits, I see that this is just moving existing code around, so it makes sense to do defer additional cleanup to a separate PR.

Indeed. I have some follow-up work that introduces a GrantableSemaphore and rewrites CountingSemaphoreGrant to be correct-by-construction (where a successfully created grant guarantees that a slot is held as it does not allow releasing or transfer). I considered closing this PR in favor of just moving straight to that, but I think it makes sense to modernize as a first step, then debate the semantics of those changes next.

threading: drop CSemaphore in favor of c++20 std::counting_semaphore #32466

Code Coverage & Benchmarks

Reviews

Conflicts