Skip to content

Commit 2b541ee

Browse files
committed
Merge bitcoin/bitcoin#34495: Replace boost signals with minimal compatible implementation
242b0eb btcsignals: use a single shared_ptr for liveness and callback (Cory Fields) b12f43a signals: remove boost::signals2 from depends and vcpkg (Cory Fields) a4b1607 signals: remove boost::signals2 mentions in linters and docs (Cory Fields) 375397e signals: remove boost includes where possible (Cory Fields) 091736a signals: re-add forward-declares to interface headers (Cory Fields) 9958f4f Revert "signals: Temporarily add boost headers to bitcoind and bitcoin-node builds" (Cory Fields) 34eabd7 signals: remove boost compatibility guards (Cory Fields) e60a0b9 signals: Add a simplified boost-compatible implementation (Cory Fields) 63c68e2 signals: add signals tests (Cory Fields) edc2978 signals: use an alias for the boost::signals2 namespace (Cory Fields) 9ade392 signals: remove forward-declare for signals (Cory Fields) 037e58b signals: use forwarding header for boost signals (Cory Fields) 2150153 signals: Temporarily add boost headers to bitcoind and bitcoin-node builds (Cory Fields) fd5e9d9 signals: Use a lambda to avoid connecting a signal to another signal (Cory Fields) Pull request description: This drops our dependency on `boost::signals2`, leaving `boost::multi_index` as the only remaining boost dependency for bitcoind. `boost::signals2` is a complex beast, but we only use a small portion of it. Namely: it's a way for multiple subscribers to connect to the same event, and the ability to later disconnect individual subscribers from that event. `btcsignals` adheres to the subset of the `boost::signals2` API that we currently use, and thus is a drop-in replacement. Rather than implementing a complex `slot` tracking class that we never used anyway (and which was much more useful in the days before std::function existed), callbacks are simply wrapped directly in `std::function`s. The new tests work with either `boost::signals2` or the new `btcsignals` implementation. Reviewers can verify functional equivalency by running the tests in the commit that introduces them against `boost::signals2`, then again with `btcsignals`. The majority of the commits in this PR are preparation and cleanup. Once `boost::signals2` is no longer needed, it is removed from depends. Additionally, a few CMake targets no longer need boost includes as they were previously only required for signals. I think this is actually pretty straightforward to review. I kept things simple, including keeping types unmovable/uncopyable where possible rather than trying to define those semantics. In doing so, the new implementation has even fewer type requirements than boost, which I believe is due to a boost bug. I've opened a PR upstream for that to attempt to maintain parity between the implementations. See individual commits for more details. Closes #26442. ACKs for top commit: fjahr: Code review ACK 242b0eb maflcko: re-review ACK 242b0eb 🎯 w0xlt: reACK 242b0eb Tree-SHA512: 9a472afa4f655624fa44493774a63b57509ad30fb61bf1d89b6d0b52000cb9a1409a5b8d515a99c76e0b26b2437c30508206c29a7dd44ea96eb1979d572cd4d4
2 parents f8ab0b7 + 242b0eb commit 2b541ee

20 files changed

Lines changed: 591 additions & 69 deletions

depends/packages/boost.mk

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ $(package)_sha256_hash = 913ca43d49e93d1b158c9862009add1518a4c665e7853b349a6492d
66
$(package)_build_subdir = build
77

88
define $(package)_set_vars
9-
$(package)_config_opts = -DBOOST_INCLUDE_LIBRARIES="multi_index;signals2;test"
9+
$(package)_config_opts = -DBOOST_INCLUDE_LIBRARIES="multi_index;test"
1010
$(package)_config_opts += -DBOOST_TEST_HEADERS_ONLY=ON
1111
$(package)_config_opts += -DBOOST_ENABLE_MPI=OFF
1212
$(package)_config_opts += -DBOOST_ENABLE_PYTHON=OFF

doc/developer-notes.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1417,9 +1417,9 @@ communication:
14171417
using TipChangedFn = std::function<void(int block_height, int64_t block_time)>;
14181418
virtual std::unique_ptr<interfaces::Handler> handleTipChanged(TipChangedFn fn) = 0;
14191419

1420-
// Bad: returns boost connection specific to local process
1420+
// Bad: returns btcsignals connection specific to local process
14211421
using TipChangedFn = std::function<void(int block_height, int64_t block_time)>;
1422-
virtual boost::signals2::scoped_connection connectTipChanged(TipChangedFn fn) = 0;
1422+
virtual btcsignals::scoped_connection connectTipChanged(TipChangedFn fn) = 0;
14231423
```
14241424
14251425
- Interface methods should not be overloaded.

src/CMakeLists.txt

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,6 @@ target_link_libraries(bitcoin_common
146146
bitcoin_util
147147
univalue
148148
secp256k1
149-
Boost::headers
150149
$<TARGET_NAME_IF_EXISTS:USDT::headers>
151150
$<$<PLATFORM_ID:Windows>:ws2_32>
152151
)
@@ -169,7 +168,6 @@ if(ENABLE_WALLET)
169168
bitcoin_wallet
170169
bitcoin_common
171170
bitcoin_util
172-
Boost::headers
173171
)
174172
install_binary_component(bitcoin-wallet HAS_MANPAGE)
175173
endif()

src/btcsignals.h

Lines changed: 257 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,257 @@
1+
// Copyright (c) The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#ifndef BITCOIN_BTCSIGNALS_H
6+
#define BITCOIN_BTCSIGNALS_H
7+
8+
#include <sync.h>
9+
10+
#include <algorithm>
11+
#include <atomic>
12+
#include <functional>
13+
#include <memory>
14+
#include <optional>
15+
#include <type_traits>
16+
#include <utility>
17+
#include <vector>
18+
19+
/**
20+
* btcsignals is a simple mechanism for signaling events to multiple subscribers.
21+
* It is api-compatible with a minimal subset of boost::signals2.
22+
*
23+
* Rather than using a custom slot type, and the features/complexity that they
24+
* imply, std::function is used to store the callbacks. Lifetime management of
25+
* the callbacks is left up to the user.
26+
*
27+
* All usage is thread-safe except for interacting with a connection while
28+
* copying/moving it on another thread.
29+
*/
30+
31+
namespace btcsignals {
32+
33+
/*
34+
* optional_last_value is the default and only supported combiner.
35+
* As such, its behavior is embedded into the signal functor.
36+
*
37+
* Because optional<void> is undefined, void must be special-cased.
38+
*/
39+
40+
template <typename T>
41+
class optional_last_value
42+
{
43+
public:
44+
using result_type = std::conditional_t<std::is_void_v<T>, void, std::optional<T>>;
45+
};
46+
47+
template <typename Signature, typename Combiner = optional_last_value<typename std::function<Signature>::result_type>>
48+
class signal;
49+
50+
/*
51+
* State object representing the liveness of a registered callback.
52+
* signal::connect() returns an enabled connection which can be held and
53+
* disabled in the future.
54+
*/
55+
class connection
56+
{
57+
template <typename Signature, typename Combiner>
58+
friend class signal;
59+
/**
60+
* Track liveness. Also serves as a tag for the constructor used by signal.
61+
*/
62+
class liveness
63+
{
64+
friend class connection;
65+
std::atomic_bool m_connected{true};
66+
67+
void disconnect() { m_connected.store(false); }
68+
public:
69+
bool connected() const { return m_connected.load(); }
70+
};
71+
72+
/**
73+
* connections have shared_ptr-like copy and move semantics.
74+
*/
75+
std::shared_ptr<liveness> m_state{};
76+
77+
/**
78+
* Only a signal can create an enabled connection.
79+
*/
80+
explicit connection(std::shared_ptr<liveness>&& state) : m_state{std::move(state)}{}
81+
82+
public:
83+
/**
84+
* The default constructor creates a connection with no associated signal
85+
*/
86+
constexpr connection() noexcept = default;
87+
88+
/**
89+
* If a callback is associated with this connection, prevent it from being
90+
* called in the future.
91+
*
92+
* If a connection is disabled as part of a signal's callback function, it
93+
* will _not_ be executed in the current signal invocation.
94+
*
95+
* Note that disconnected callbacks are not removed from their owning
96+
* signals here. They are garbage collected in signal::connect().
97+
*/
98+
void disconnect()
99+
{
100+
if (m_state) {
101+
m_state->disconnect();
102+
}
103+
}
104+
105+
/**
106+
* Returns true if this connection was created by a signal and has not been
107+
* disabled.
108+
*/
109+
bool connected() const
110+
{
111+
return m_state && m_state->connected();
112+
}
113+
};
114+
115+
/*
116+
* RAII-style connection management
117+
*/
118+
class scoped_connection
119+
{
120+
connection m_conn;
121+
122+
public:
123+
scoped_connection(connection rhs) noexcept : m_conn{std::move(rhs)} {}
124+
125+
scoped_connection(scoped_connection&&) noexcept = default;
126+
scoped_connection& operator=(scoped_connection&&) noexcept = default;
127+
128+
/**
129+
* For simplicity, disable copy assignment and construction.
130+
*/
131+
scoped_connection& operator=(const scoped_connection&) = delete;
132+
scoped_connection(const scoped_connection&) = delete;
133+
134+
void disconnect()
135+
{
136+
m_conn.disconnect();
137+
}
138+
139+
~scoped_connection()
140+
{
141+
disconnect();
142+
}
143+
};
144+
145+
/*
146+
* Functor for calling zero or more connected callbacks
147+
*/
148+
template <typename Signature, typename Combiner>
149+
class signal
150+
{
151+
using function_type = std::function<Signature>;
152+
153+
static_assert(std::is_same_v<Combiner, optional_last_value<typename function_type::result_type>>, "only the optional_last_value combiner is supported");
154+
155+
/*
156+
* Helper struct for maintaining a callback and its associated connection liveness
157+
*/
158+
struct connection_holder : connection::liveness {
159+
template <typename Callable>
160+
connection_holder(Callable&& callback) : m_callback{std::forward<Callable>(callback)}
161+
{
162+
}
163+
164+
const function_type m_callback;
165+
};
166+
167+
mutable Mutex m_mutex;
168+
169+
std::vector<std::shared_ptr<connection_holder>> m_connections GUARDED_BY(m_mutex){};
170+
171+
public:
172+
using result_type = Combiner::result_type;
173+
174+
constexpr signal() noexcept = default;
175+
~signal() = default;
176+
177+
/*
178+
* For simplicity, disable all moving/copying/assigning.
179+
*/
180+
signal(const signal&) = delete;
181+
signal(signal&&) = delete;
182+
signal& operator=(const signal&) = delete;
183+
signal& operator=(signal&&) = delete;
184+
185+
/*
186+
* Execute all enabled callbacks for the signal. Rather than allowing for
187+
* custom combiners, the behavior of optional_last_value is hard-coded
188+
* here. Return the value of the last executed callback, or nullopt if none
189+
* were executed.
190+
*
191+
* Callbacks which return void require special handling.
192+
*
193+
* In order to avoid locking during the callbacks, the list of callbacks is
194+
* cached before they are called. This allows a callback to call connect(),
195+
* but the newly connected callback will not be run during the current
196+
* signal invocation.
197+
*
198+
* Note that the parameters are accepted as universal references, though
199+
* they are not perfectly forwarded as that could cause a use-after-move if
200+
* more than one callback is enabled.
201+
*/
202+
template <typename... Args>
203+
result_type operator()(Args&&... args) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
204+
{
205+
std::vector<std::shared_ptr<connection_holder>> connections;
206+
{
207+
LOCK(m_mutex);
208+
connections = m_connections;
209+
}
210+
if constexpr (std::is_void_v<result_type>) {
211+
for (const auto& connection : connections) {
212+
if (connection->connected()) {
213+
connection->m_callback(args...);
214+
}
215+
}
216+
} else {
217+
result_type ret{std::nullopt};
218+
for (const auto& connection : connections) {
219+
if (connection->connected()) {
220+
ret.emplace(connection->m_callback(args...));
221+
}
222+
}
223+
return ret;
224+
}
225+
}
226+
227+
/*
228+
* Connect a new callback to the signal. A forwarding callable accepts
229+
* anything that can be stored in a std::function.
230+
*/
231+
template <typename Callable>
232+
connection connect(Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
233+
{
234+
LOCK(m_mutex);
235+
236+
// Garbage-collect disconnected connections to prevent unbounded growth
237+
std::erase_if(m_connections, [](const auto& holder) { return !holder->connected(); });
238+
239+
const auto& entry = m_connections.emplace_back(std::make_shared<connection_holder>(std::forward<Callable>(func)));
240+
return connection(entry);
241+
}
242+
243+
/*
244+
* Returns true if there are no enabled callbacks
245+
*/
246+
[[nodiscard]] bool empty() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
247+
{
248+
LOCK(m_mutex);
249+
return std::ranges::none_of(m_connections, [](const auto& holder) {
250+
return holder->connected();
251+
});
252+
}
253+
};
254+
255+
} // namespace btcsignals
256+
257+
#endif // BITCOIN_BTCSIGNALS_H

src/common/interfaces.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,10 @@
22
// Distributed under the MIT software license, see the accompanying
33
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
44

5+
#include <btcsignals.h>
56
#include <interfaces/echo.h>
67
#include <interfaces/handler.h>
78

8-
#include <boost/signals2/connection.hpp>
99
#include <memory>
1010
#include <utility>
1111

@@ -23,11 +23,11 @@ class CleanupHandler : public interfaces::Handler
2323
class SignalHandler : public interfaces::Handler
2424
{
2525
public:
26-
explicit SignalHandler(boost::signals2::connection connection) : m_connection(std::move(connection)) {}
26+
explicit SignalHandler(btcsignals::connection connection) : m_connection(std::move(connection)) {}
2727

2828
void disconnect() override { m_connection.disconnect(); }
2929

30-
boost::signals2::scoped_connection m_connection;
30+
btcsignals::scoped_connection m_connection;
3131
};
3232

3333
class EchoImpl : public interfaces::Echo
@@ -44,7 +44,7 @@ std::unique_ptr<Handler> MakeCleanupHandler(std::function<void()> cleanup)
4444
return std::make_unique<common::CleanupHandler>(std::move(cleanup));
4545
}
4646

47-
std::unique_ptr<Handler> MakeSignalHandler(boost::signals2::connection connection)
47+
std::unique_ptr<Handler> MakeSignalHandler(btcsignals::connection connection)
4848
{
4949
return std::make_unique<common::SignalHandler>(std::move(connection));
5050
}

src/init.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include <addrman.h>
1313
#include <banman.h>
1414
#include <blockfilter.h>
15+
#include <btcsignals.h>
1516
#include <chain.h>
1617
#include <chainparams.h>
1718
#include <chainparamsbase.h>
@@ -112,8 +113,6 @@
112113
#include <sys/stat.h>
113114
#endif
114115

115-
#include <boost/signals2/signal.hpp>
116-
117116
#ifdef ENABLE_ZMQ
118117
#include <zmq/zmqabstractnotifier.h>
119118
#include <zmq/zmqnotificationinterface.h>

src/interfaces/handler.h

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,9 @@
88
#include <functional>
99
#include <memory>
1010

11-
namespace boost {
12-
namespace signals2 {
13-
class connection;
14-
} // namespace signals2
15-
} // namespace boost
11+
namespace btcsignals {
12+
class connection;
13+
} // namespace btcsignals
1614

1715
namespace interfaces {
1816

@@ -28,8 +26,8 @@ class Handler
2826
virtual void disconnect() = 0;
2927
};
3028

31-
//! Return handler wrapping a boost signal connection.
32-
std::unique_ptr<Handler> MakeSignalHandler(boost::signals2::connection connection);
29+
//! Return handler wrapping a btcsignals connection.
30+
std::unique_ptr<Handler> MakeSignalHandler(btcsignals::connection connection);
3331

3432
//! Return handler wrapping a cleanup function.
3533
std::unique_ptr<Handler> MakeCleanupHandler(std::function<void()> cleanup);

0 commit comments

Comments
 (0)