Skip to content

refactor: rename EventLoop::m_num_clients to m_num_refs - #302

Merged
ryanofsky merged 1 commit into
bitcoin-core:masterfrom
Sjors:2026/07/num-clients
Jul 5, 2026
Merged

refactor: rename EventLoop::m_num_clients to m_num_refs#302
ryanofsky merged 1 commit into
bitcoin-core:masterfrom
Sjors:2026/07/num-clients

Conversation

@Sjors

@Sjors Sjors commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Rename the variable and document what it counts, so the loop() exit condition in EventLoop::done() is not misread as "exit when no clients are connected".

I found myself confused by this while reviewing #269.

@Sjors
refactor: rename EventLoop::m_num_clients to m_num_refs
d499830 
Rename the variable and document what it counts, so the loop() exit
condition in EventLoop::done() is not misread as "exit when no
clients are connected".
@DrahtBot

DrahtBot commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK ViniciusCestarii, ryanofsky

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #288 (Create support branch for CI scripts, documentation, and examples by ryanofsky)
  • #274 (Add nonunix platform support by ryanofsky)

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.

@Sjors Sjors mentioned this pull request Jul 2, 2026
Merged
ViniciusCestarii
ViniciusCestarii reviewed Jul 3, 2026
View reviewed changes

@ViniciusCestarii ViniciusCestarii left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ACK d499830, I agree this name is clearer.

Comment thread include/mp/proxy-io.h
//! Run event loop. Does not return until shutdown. This should only be
//! called once from the m_thread_id thread. This will block until
//! the m_num_clients reference count is 0.
//! the m_num_refs reference count is 0.

@ViniciusCestarii ViniciusCestarii Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In "refactor: rename EventLoop::m_num_clients to m_num_refs" d499830

nit: "the m_num_refs reference count" is a bit redundant now because "refs" already means references. Could be renamed to "the m_num_refs count is 0" (unrelated but this condition is also incomplete: loop() exits on done(), m_num_refs == 0 and m_async_fns is empty).

This was referenced Jul 4, 2026
Open
Merged
ryanofsky
ryanofsky approved these changes Jul 5, 2026
View reviewed changes

@ryanofsky ryanofsky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code review ACK d499830. Thanks for the rename and comment!

This was previously discussed #107 (comment)

Probably the word "client" should be replaced with "use count" here (in num_clients addClient removeClient). The word "client" is supposed to mean "client of the event loop" but that seems unnecessarily confusing because the word client is overloaded so much.

And #107 (comment)

I didn't find this that confusing to be honest, and just by looking at the names was assuming "client of the event loop".

But the word was used more places then so renaming is easier now and makes sense.

@ryanofsky
ryanofsky merged commit 16bf05d into bitcoin-core:master Jul 5, 2026
13 checks passed
@ryanofsky ryanofsky mentioned this pull request Jul 5, 2026
Merged
fanquake pushed a commit to bitcoin/bitcoin that referenced this pull request Jul 7, 2026
@ryanofsky
Squashed 'src/ipc/libmultiprocess/' changes from 3edbe8f..16bf05d
6b0a907 
16bf05d Merge bitcoin-core/libmultiprocess#302: refactor: rename EventLoop::m_num_clients to m_num_refs
dd537da Merge bitcoin-core/libmultiprocess#301: test: recursive async IPC calls and cleanups
400291d Merge bitcoin-core/libmultiprocess#299: ci: remove libevent from Core CIs
092be51 Merge bitcoin-core/libmultiprocess#285: Add ReadList helper
5b61788 Merge bitcoin-core/libmultiprocess#283: Add `makePool` method on `ThreadMap`
d499830 refactor: rename EventLoop::m_num_clients to m_num_refs
6450345 type: reserve first when reading std::unordered_set
4d0f8db proxy: add ReadList helper and dedup map/set/vector read handlers
0e49d91 Add `makePool` method on `ThreadMap`
5519f7f test: recursive async IPC calls
a29ceff ci: remove libevent from Core CIs
8412fcd Merge bitcoin-core/libmultiprocess#295: Mark Waiter m_cv as guarded by m_mutex
1593ee2 Merge bitcoin-core/libmultiprocess#294: test: Add passDouble smoke test
9885d7d Merge bitcoin-core/libmultiprocess#286: proxy-client: fix TSan data race in clientDestroy
fa35501 Mark Waiter m_cv as guarded by m_mutex
faaedb1 test: Add passDouble smoke test
733c643 Merge bitcoin-core/libmultiprocess#292: type-number: fix clang-tidy modernize-use-nullptr
9cc3479 Merge bitcoin-core/libmultiprocess#291: cmake: Add `mp_headers` custom target
201abd9 Merge bitcoin-core/libmultiprocess#289: cmake: make target_capnp_sources use CURRENT dirs
99820c8 Merge bitcoin-core/libmultiprocess#279: doc: Add comments to FIELD_* constants in proxy.h
73b9855 Merge bitcoin-core/libmultiprocess#278: doc: Fix and expand design.md
e7e91b2 Merge bitcoin-core/libmultiprocess#277: Add std::unordered_set support and a helper BuildList to dedup list build handlers
91a951f tidy fix: modernize-use-nullptr
16362f4 cmake: Add `mp_headers` custom target
615a94f cmake: document ONLY_CAPNP option in target_capnp_sources
90982f7 mpgen: iwyu changes required by previous commit
25bb3e6 proxy-client: fix TSan data race in clientDestroy
620f297 cmake: make target_capnp_sources use CURRENT dirs
9de4b88 test: use camelCase + $Proxy.name for FooStruct fields
011b917 type: add std::unordered_set support
20d19b9 proxy: add BuildList helper and dedup map/set/vector build handlers
e863c6c doc: Add comments to FIELD_* constants in proxy.h
18db0ab doc: Fix and expand design.md
61de697 Merge bitcoin-core/libmultiprocess#273: proxy-client: tolerate exceptions from remote destroy during cleanup
9cec9d6 Merge bitcoin-core/libmultiprocess#243: mpgen: support primitive std::optional struct fields
4aaff11 Merge bitcoin-core/libmultiprocess#238: cmake, ci: updates for recent nixpkgs
2ac55a5 Merge bitcoin-core/libmultiprocess#218: Better error and log messages
6de92e1 proxy-client: tolerate exceptions from remote destroy during cleanup
90be835 test: regression for ~ProxyClient destroy after peer disconnect
3c69d12 Merge bitcoin-core/libmultiprocess#260: event loop: tolerate unexpected exceptions in `post()` callbacks
b8a48c6 event loop: tolerate unexpected exceptions in `post()` callbacks
f787863 Merge bitcoin-core/libmultiprocess#270: doc: Bump version 10 > 11
a22f602 doc: Bump version 10 > 11
4eae445 debug: Add TypeName() function and log statements for Proxy objects being created and destroyed
f326c5b logging: Add better logging on IPC server-side failures
6dbfa56 mpgen: support primitive std::optional struct fields
8d1277d mpgen refactor: add AccessorType function
db716bb mpgen refactor: Move field handling code to FieldList class
db7acb3 ci: Fix shell.nix compatibility with CMake 4.0
91a7759 cmake: Fix IWYU in nix by adding CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES

git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: 16bf05d
fanquake added a commit to bitcoin/bitcoin that referenced this pull request Jul 7, 2026
@fanquake
Merge #35661: Update libmultiprocess subtree to add `ThreadMap.makePo…
ec5edcd 
…ol` method

6b0a907 Squashed 'src/ipc/libmultiprocess/' changes from 3edbe8f..16bf05d (Ryan Ofsky)

Pull request description:

  The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh).

  Changes since last subtree update (#34977):

  - Adds `makePool` method on `ThreadMap` to support thread pool routing, allowing requests without a specific client thread to be dispatched to a pool using a shortest-queue strategy ([#283](bitcoin-core/libmultiprocess#283)).
  - Adds `std::unordered_set` support, a `BuildList` helper, and a `ReadList` helper to reduce duplication in list build and read handlers ([#277](bitcoin-core/libmultiprocess#277), [#285](bitcoin-core/libmultiprocess#285)).
  - Adds support for translating C++ `std::optional<T>` struct fields to pairs of `T` + `hasT :Bool` Cap'n Proto struct fields, allowing unset optional primitive fields to be represented ([#243](bitcoin-core/libmultiprocess#243)).
  - Produces more readable log output for Proxy object lifecycle events and IPC server-side failures ([#218](bitcoin-core/libmultiprocess#218)).
  - Handles exceptions thrown by `destroy` methods by logging instead of aborting ([#273](bitcoin-core/libmultiprocess#273)). This can prevent server crashes when non-libmultiprocess clients disconnect without destroying objects, in the case where a server object owns client objects and the server destructor tries to call the disconnected client to free them ([#219](bitcoin-core/libmultiprocess#219)).
  - Handles unexpected exceptions thrown by callbacks (that should never happen) by logging errors instead of deadlocking ([#260](bitcoin-core/libmultiprocess#260)).
  - Fixes a rare mptest hang on musl builds caused by a lost wakeup bug in `Waiter` ([#295](bitcoin-core/libmultiprocess#295)).
  - Fixes a race condition in a log print detected by TSan ([#286](bitcoin-core/libmultiprocess#286)).
  - Build improvements: makes `target_capnp_sources` work correctly when libmultiprocess is used as a CMake subproject ([#289](bitcoin-core/libmultiprocess#289)), adds `mp_headers` target for better lint tool support ([#291](bitcoin-core/libmultiprocess#291)), and fixes compatibility with recent Nix and CMake 4.0 ([#238](bitcoin-core/libmultiprocess#238)).
  - Test, CI, documentation, and minor code improvements: design document corrections ([#278](bitcoin-core/libmultiprocess#278)), field constant comments ([#279](bitcoin-core/libmultiprocess#279)), clang-tidy fix ([#292](bitcoin-core/libmultiprocess#292)), new smoke test for double-precision float values ([#294](bitcoin-core/libmultiprocess#294)), new test for recursive async IPC calls ([#301](bitcoin-core/libmultiprocess#301)), removal of libevent from Core CI builds ([#299](bitcoin-core/libmultiprocess#299)), and rename of `EventLoop::m_num_clients` to `m_num_refs` ([#302](bitcoin-core/libmultiprocess#302)).

ACKs for top commit:
  fanquake:
    ACK 02afa66
  hebasto:
    ACK 02afa66.

Tree-SHA512: ef81a951c971f328a0a98436030467eeea30925eb6016eafd9bc7a25726c87628a852bbb1d84b88bce340aeea2bed25c65bc55db1168ebcb850628cd18808883
Kino1994 pushed a commit to Kino1994/bitcoin-full-history that referenced this pull request Jul 10, 2026
@fanquake
Merge bitcoin/bitcoin#35661: Update libmultiprocess subtree to add `T…
bd97050 
…hreadMap.makePool` method

6b0a907 Squashed 'src/ipc/libmultiprocess/' changes from 3edbe8f67c1..16bf05dea02 (Ryan Ofsky)

Pull request description:

  The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh).

  Changes since last subtree update (#34977):

  - Adds `makePool` method on `ThreadMap` to support thread pool routing, allowing requests without a specific client thread to be dispatched to a pool using a shortest-queue strategy ([#283](bitcoin-core/libmultiprocess#283)).
  - Adds `std::unordered_set` support, a `BuildList` helper, and a `ReadList` helper to reduce duplication in list build and read handlers ([#277](bitcoin-core/libmultiprocess#277), [#285](bitcoin-core/libmultiprocess#285)).
  - Adds support for translating C++ `std::optional<T>` struct fields to pairs of `T` + `hasT :Bool` Cap'n Proto struct fields, allowing unset optional primitive fields to be represented ([#243](bitcoin-core/libmultiprocess#243)).
  - Produces more readable log output for Proxy object lifecycle events and IPC server-side failures ([#218](bitcoin-core/libmultiprocess#218)).
  - Handles exceptions thrown by `destroy` methods by logging instead of aborting ([#273](bitcoin-core/libmultiprocess#273)). This can prevent server crashes when non-libmultiprocess clients disconnect without destroying objects, in the case where a server object owns client objects and the server destructor tries to call the disconnected client to free them ([#219](bitcoin-core/libmultiprocess#219)).
  - Handles unexpected exceptions thrown by callbacks (that should never happen) by logging errors instead of deadlocking ([#260](bitcoin-core/libmultiprocess#260)).
  - Fixes a rare mptest hang on musl builds caused by a lost wakeup bug in `Waiter` ([#295](bitcoin-core/libmultiprocess#295)).
  - Fixes a race condition in a log print detected by TSan ([#286](bitcoin-core/libmultiprocess#286)).
  - Build improvements: makes `target_capnp_sources` work correctly when libmultiprocess is used as a CMake subproject ([#289](bitcoin-core/libmultiprocess#289)), adds `mp_headers` target for better lint tool support ([#291](bitcoin-core/libmultiprocess#291)), and fixes compatibility with recent Nix and CMake 4.0 ([#238](bitcoin-core/libmultiprocess#238)).
  - Test, CI, documentation, and minor code improvements: design document corrections ([#278](bitcoin-core/libmultiprocess#278)), field constant comments ([#279](bitcoin-core/libmultiprocess#279)), clang-tidy fix ([#292](bitcoin-core/libmultiprocess#292)), new smoke test for double-precision float values ([#294](bitcoin-core/libmultiprocess#294)), new test for recursive async IPC calls ([#301](bitcoin-core/libmultiprocess#301)), removal of libevent from Core CI builds ([#299](bitcoin-core/libmultiprocess#299)), and rename of `EventLoop::m_num_clients` to `m_num_refs` ([#302](bitcoin-core/libmultiprocess#302)).

ACKs for top commit:
  fanquake:
    ACK cd09e20
  hebasto:
    ACK cd09e20.

Tree-SHA512: ef81a951c971f328a0a98436030467eeea30925eb6016eafd9bc7a25726c87628a852bbb1d84b88bce340aeea2bed25c65bc55db1168ebcb850628cd18808883
Sjors added a commit to Sjors/sv2-tp that referenced this pull request Jul 21, 2026
@Sjors
Squashed 'src/ipc/libmultiprocess/' changes from 8412fcdc65..e8de5c7b68
a6b200c 
e8de5c7b68 Merge bitcoin-core/libmultiprocess#305: refactor: memcpy to std::ranges::copy to work around ubsan warn
9307e68e5a Merge bitcoin-core/libmultiprocess#306: doc: Bump version 12 > 13
fac7b9b7f6 refactor: memcpy to std::ranges::copy to work around ubsan warn
1bd7025609 Merge bitcoin-core/libmultiprocess#297: test: add map serialization round-trip coverage
438fdd243d doc: Bump version 12 > 13
28e056576a Merge bitcoin-core/libmultiprocess#269: proxy: add local connection limit to ListenConnections
39a10ce895 proxy: add local connection limit to ListenConnections()
43172f52d9 test: add dedicated ListenConnections coverage
033f812195 doc/version: Bump version 11 > 12
463d073cb8 test: rename vBool to vector_bool
16bf05dea0 Merge bitcoin-core/libmultiprocess#302: refactor: rename EventLoop::m_num_clients to m_num_refs
dd537da9e4 Merge bitcoin-core/libmultiprocess#301: test: recursive async IPC calls and cleanups
400291de00 Merge bitcoin-core/libmultiprocess#299: ci: remove libevent from Core CIs
092be515ad Merge bitcoin-core/libmultiprocess#285: Add ReadList helper
5b617880c5 Merge bitcoin-core/libmultiprocess#283: Add `makePool` method on `ThreadMap`
d499830415 refactor: rename EventLoop::m_num_clients to m_num_refs
6450345c98 type: reserve first when reading std::unordered_set
4d0f8db5f9 proxy: add ReadList helper and dedup map/set/vector read handlers
0e49d91186 Add `makePool` method on `ThreadMap`
5519f7f948 test: recursive async IPC calls
a29ceff40b ci: remove libevent from Core CIs
85df233845 test: add mapStringInt to foo.capnp to cover map serialization and deserialization

git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: e8de5c7b68e0ae21c94ae92aa22e5c3b213f9c12
Sjors added a commit to Sjors/sv2-tp that referenced this pull request Jul 21, 2026
@Sjors
Squashed 'src/ipc/libmultiprocess/' changes from 8412fcdc65..e8de5c7b68
79bd483 
e8de5c7b68 Merge bitcoin-core/libmultiprocess#305: refactor: memcpy to std::ranges::copy to work around ubsan warn
9307e68e5a Merge bitcoin-core/libmultiprocess#306: doc: Bump version 12 > 13
fac7b9b7f6 refactor: memcpy to std::ranges::copy to work around ubsan warn
1bd7025609 Merge bitcoin-core/libmultiprocess#297: test: add map serialization round-trip coverage
438fdd243d doc: Bump version 12 > 13
28e056576a Merge bitcoin-core/libmultiprocess#269: proxy: add local connection limit to ListenConnections
39a10ce895 proxy: add local connection limit to ListenConnections()
43172f52d9 test: add dedicated ListenConnections coverage
033f812195 doc/version: Bump version 11 > 12
463d073cb8 test: rename vBool to vector_bool
16bf05dea0 Merge bitcoin-core/libmultiprocess#302: refactor: rename EventLoop::m_num_clients to m_num_refs
dd537da9e4 Merge bitcoin-core/libmultiprocess#301: test: recursive async IPC calls and cleanups
400291de00 Merge bitcoin-core/libmultiprocess#299: ci: remove libevent from Core CIs
092be515ad Merge bitcoin-core/libmultiprocess#285: Add ReadList helper
5b617880c5 Merge bitcoin-core/libmultiprocess#283: Add `makePool` method on `ThreadMap`
d499830415 refactor: rename EventLoop::m_num_clients to m_num_refs
6450345c98 type: reserve first when reading std::unordered_set
4d0f8db5f9 proxy: add ReadList helper and dedup map/set/vector read handlers
0e49d91186 Add `makePool` method on `ThreadMap`
5519f7f948 test: recursive async IPC calls
a29ceff40b ci: remove libevent from Core CIs
85df233845 test: add mapStringInt to foo.capnp to cover map serialization and deserialization

git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: e8de5c7b68e0ae21c94ae92aa22e5c3b213f9c12
Kino1994 pushed a commit to Kino1994/bitcoin-full-history that referenced this pull request Aug 19, 2026
@fanquake
Merge bitcoin/bitcoin#35661: Update libmultiprocess subtree to add `T…
15672c5 
…hreadMap.makePool` method

6b0a907 Squashed 'src/ipc/libmultiprocess/' changes from 3edbe8f67c1..16bf05dea02 (Ryan Ofsky)

Pull request description:

  The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh).

  Changes since last subtree update (#34977):

  - Adds `makePool` method on `ThreadMap` to support thread pool routing, allowing requests without a specific client thread to be dispatched to a pool using a shortest-queue strategy ([#283](bitcoin-core/libmultiprocess#283)).
  - Adds `std::unordered_set` support, a `BuildList` helper, and a `ReadList` helper to reduce duplication in list build and read handlers ([#277](bitcoin-core/libmultiprocess#277), [#285](bitcoin-core/libmultiprocess#285)).
  - Adds support for translating C++ `std::optional<T>` struct fields to pairs of `T` + `hasT :Bool` Cap'n Proto struct fields, allowing unset optional primitive fields to be represented ([#243](bitcoin-core/libmultiprocess#243)).
  - Produces more readable log output for Proxy object lifecycle events and IPC server-side failures ([#218](bitcoin-core/libmultiprocess#218)).
  - Handles exceptions thrown by `destroy` methods by logging instead of aborting ([#273](bitcoin-core/libmultiprocess#273)). This can prevent server crashes when non-libmultiprocess clients disconnect without destroying objects, in the case where a server object owns client objects and the server destructor tries to call the disconnected client to free them ([#219](bitcoin-core/libmultiprocess#219)).
  - Handles unexpected exceptions thrown by callbacks (that should never happen) by logging errors instead of deadlocking ([#260](bitcoin-core/libmultiprocess#260)).
  - Fixes a rare mptest hang on musl builds caused by a lost wakeup bug in `Waiter` ([#295](bitcoin-core/libmultiprocess#295)).
  - Fixes a race condition in a log print detected by TSan ([#286](bitcoin-core/libmultiprocess#286)).
  - Build improvements: makes `target_capnp_sources` work correctly when libmultiprocess is used as a CMake subproject ([#289](bitcoin-core/libmultiprocess#289)), adds `mp_headers` target for better lint tool support ([#291](bitcoin-core/libmultiprocess#291)), and fixes compatibility with recent Nix and CMake 4.0 ([#238](bitcoin-core/libmultiprocess#238)).
  - Test, CI, documentation, and minor code improvements: design document corrections ([#278](bitcoin-core/libmultiprocess#278)), field constant comments ([#279](bitcoin-core/libmultiprocess#279)), clang-tidy fix ([#292](bitcoin-core/libmultiprocess#292)), new smoke test for double-precision float values ([#294](bitcoin-core/libmultiprocess#294)), new test for recursive async IPC calls ([#301](bitcoin-core/libmultiprocess#301)), removal of libevent from Core CI builds ([#299](bitcoin-core/libmultiprocess#299)), and rename of `EventLoop::m_num_clients` to `m_num_refs` ([#302](bitcoin-core/libmultiprocess#302)).

ACKs for top commit:
  fanquake:
    ACK 02afa66169b5dc58d0fc6f608d0c6f4facefd5ec
  hebasto:
    ACK 02afa66169b5dc58d0fc6f608d0c6f4facefd5ec.

Tree-SHA512: ef81a951c971f328a0a98436030467eeea30925eb6016eafd9bc7a25726c87628a852bbb1d84b88bce340aeea2bed25c65bc55db1168ebcb850628cd18808883
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes
+1 more reviewer
@ViniciusCestarii ViniciusCestarii ViniciusCestarii left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Sjors @DrahtBot @ryanofsky @ViniciusCestarii