btcsignals: destroy callbacks on disconnect #36011

ryanofsky commented at 4:28 PM on August 18, 2026: contributor

Currently callback functions connected to signals do not get freed when their connection::disconnect method is called or when their associated scoped_connection is destroyed. They live as long as the signal does, even if they will never be called, and can only get garbage collected if new connections are added.

This behavior is different that previous boost signals behavior and causes a deadlock on GUI shutdown in #10102 because when the node registers signal callbacks (handleInitMessage, etc) that get forwarded to the GUI, the callbacks own mp::EventLoopRef references, and if the callbacks aren't freed, the node process can't shut down.

This change restores boost signals behavior freeing callbacks when they are disconnected or when scoped_connection objects are destroyed to avoid this problem and make callback lifetimes more predictable.

test: characterize btcsignals callback destruction timing

Add tests pinning down when a callback object (and any state it owns)
is destroyed. Currently disconnect() only disables the callback: the
object stays alive until a later connect() call on the same signal
garbage collects it and every connection handle referencing it has been
released; with no connection handle held it lives until the signal
itself is destroyed.

This differs from boost::signals2, which btcsignals aims to be
api-compatible with: there, disconnecting destroys the slot's function
object as soon as no emission is using it.

No behavior change; these tests document the status quo so the next
commit changing the destruction timing has a clear before/after diff.

This change was written with Claude Fable 5.

060f8f0cf2

btcsignals: destroy callbacks eagerly on disconnect

Previously a disconnected callback and any state it owned could stay
alive indefinitely: destruction required both a later connect() call on
the same signal to garbage collect it and release of every connection
handle referencing it. That made destruction timing unpredictable for
callbacks that own resources whose release has side effects, and it
diverges from boost::signals2, which destroys a disconnected slot's
function object as soon as no emission is running it. (The concrete
fallout was in multiprocess bitcoin-gui, where node notification
callbacks own IPC proxy objects: their deferred destruction kept
bitcoin-node from exiting and hung interface_gui.py until timeout.)

Make disconnect() destroy the callback, deferring only while a
concurrent emission is mid-call into it, matching boost::signals2
semantics. The previous commit's characterization tests are updated;
their diff shows the ownership change.

This change was written with Claude Fable 5.

8bd1cda004

btcsignals: fall back to atomic_load/store without __cpp_lib_atomic_shared_ptr

std::atomic<shared_ptr<T>> is a C++20 library feature, but library
implementations are not required to ship all C++20 features at once;
__cpp_lib_atomic_shared_ptr is the feature-test macro that indicates its
presence. Without it, the generic std::atomic<T> requires trivially copyable
T — shared_ptr is not, causing build failures on macOS (Xcode 16.2),
FreeBSD 15.1, and the MSan/TSan custom libc++ builds in CI.

Fall back to the C++11 atomic_load/atomic_store free functions, which are
specifically overloaded for shared_ptr, through a small atomic_shared_ptr
wrapper with the same load() and store() members. The fallback is intended
to be temporary: those functions are deprecated in C++20 and will eventually
be removed, so this commit can be reverted once all CI platforms carry the
C++20 specialization.

This change was written with Claude Sonnet 4.6.

47a5fc7671

DrahtBot commented at 4:28 PM on August 18, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36011
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36011.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36011#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	sedited, hodlinator

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19461 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/19461.svg"> (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/19460.svg"> (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#10102 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/10102.svg"> (Multiprocess bitcoin 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.

fanquake requested review from theuni on Aug 18, 2026

DrahtBot added the label CI failed on Aug 18, 2026

maflcko commented at 10:26 AM on August 19, 2026: member

Heh, I really loved the simple garbage collect logic. Too bad it doesn't work for the mp-gui. I guess this can slip the next major release, as there is no rush, right?

DrahtBot removed the label CI failed on Aug 19, 2026

ryanofsky commented at 12:24 PM on August 19, 2026: contributor

Yeah no rush, and there are also some potential simplifications this change could allow. This PR is trying to make behavior of btcsignals more predictable and free the external callbacks after they can no longer be called. But internal improvements might be possible too:

connection::liveness::m_connected variable could maybe be dropped. It now seems redundant with the m_owner variable.
Maybe the garbage collection loop can be dropped and replaced with an m_owner custom deleter that deletes entries from m_connections
I'm not sure if m_callback is actually necessary. Maybe the callback pointer could be derived from m_owner with a cast.

sedited commented at 9:40 AM on October 6, 2026: contributor

Approach ACK

I'm not sure if m_callback is actually necessary. Maybe the callback pointer could be derived from m_owner with a cast.

I think I'd prefer this done here. It took me a couple of reads to understand why this is done at all. How about:

diff --git a/src/util/btcsignals.h b/src/util/btcsignals.h
index 367fcfe851d..74f455fd6f1 100644
--- a/src/util/btcsignals.h
+++ b/src/util/btcsignals.h
@@ -187 +186,0 @@ class signal
-            m_callback = owner.get();
@@ -193,5 +192,5 @@ class signal
-        std::shared_ptr<const void> owner() const { return m_owner.load(); }
-
-        //! Raw pointer to the callback, only valid while holding a shared_ptr
-        //! returned by owner().
-        const function_type* m_callback;
+        std::shared_ptr<const function_type> owner() const
+        {
+            auto owner = m_owner.load();
+            return std::static_pointer_cast<const function_type>(std::move(owner));
+        }
@@ -249 +248 @@ public:
-                    (*connection->m_callback)(args...);
+                    (*owner)(args...);
@@ -260 +259 @@ public:
-                    ret |= (*connection->m_callback)(args...);
+                    ret |= (*owner)(args...);

in src/util/btcsignals.h:135 in 6a3d490d82 outdated

 128 | @@ -94,8 +129,10 @@ class connection
 129 |       * If a connection is disabled as part of a signal's callback function, it
 130 |       * will _not_ be executed in the current signal invocation.
 131 |       *
 132 | -     * Note that disconnected callbacks are not removed from their owning
 133 | -     * signals here. They are garbage collected in signal::connect().
 134 | +     * The callback object is destroyed here (or when the last concurrently
 135 | +     * executing invocation of it completes). The empty holder entry is not
 136 | +     * removed from the owning signal; it is garbage collected in
 137 | +     * signal::connect().

ryanofsky commented at 9:10 AM on October 14, 2026:

re: #36011 (comment)

in commit "btcsignals: destroy callbacks eagerly on disconnect" (c61fa5768fdc291ab0fdbb71b1ab240cf1e6485e)

note: i didn't follow up on my earlier idea of replacing the connect() garbage collection loop with a custom deleter that removes entries from m_connections. the deleter would need a pointer back to the signal, and since connection handles can outlive the signal, a late disconnect() would run it against a destroyed signal unless the signal's state were moved behind a shared_ptr/weak_ptr. it would also run on whichever thread drops the last reference, which could be a thread emitting the signal or the signal destructor, and would need to lock m_mutex there. that is more complexity than the loop, which now costs little: a disconnected entry no longer holds the callback, only the small liveness object, and the next connect() call removes it.

ryanofsky force-pushed on Oct 14, 2026

ryanofsky commented at 9:47 AM on October 14, 2026: contributor

Thanks for the review!

Updated 6a3d490d826a50da6ac812cbf204320d5780f7f3 -> 47a5fc767162ff1e141113f2ff68e42435310d47 (pr/sigdest.1 -> pr/sigdest.2, compare) dropping redundant m_connected and m_callback members as suggested by sedited, adding a test for disconnecting a callback while it runs on another thread, and moving the atomic_load/atomic_store fallback into one #ifdef block so it is easy to revert later

sedited approved

sedited commented at 2:54 AM on October 15, 2026: contributor

ACK 47a5fc767162ff1e141113f2ff68e42435310d47

sedited requested review from hodlinator on Oct 19, 2026

in src/util/btcsignals.h:204 in 47a5fc7671

 200 | @@ -157,11 +201,17 @@ class signal
 201 |       */
 202 |      struct connection_holder : connection::liveness {
 203 |          template <typename Callable>
 204 | -        connection_holder(Callable&& callback) : m_callback{std::forward<Callable>(callback)}
 205 | +        connection_holder(Callable&& callback)

hodlinator commented at 12:51 PM on October 21, 2026:

nit: Might as well make ctor explicit:

        explicit connection_holder(Callable&& callback)

Same for atomic_shared_ptr(std::shared_ptr<T> ptr).

in src/test/btcsignals_tests.cpp:110 in 47a5fc7671

 105 | +    // handle is still held.
 106 | +    btcsignals::signal<void()> sig1;
 107 | +    auto state1{std::make_shared<int>(0)};
 108 | +    std::weak_ptr<int> weak1{state1};
 109 | +    auto conn1{sig1.connect([state1 = std::move(state1)] {})};
 110 | +    conn1.disconnect();

hodlinator commented at 1:03 PM on October 21, 2026:

nit: Could verify state between connect/disconnect calls:

    auto conn1{sig1.connect([state1 = std::move(state1)] {})};
    BOOST_CHECK(!weak1.expired());
    conn1.disconnect();

hodlinator approved

hodlinator commented at 1:42 PM on October 21, 2026: contributor

light ACK 47a5fc767162ff1e141113f2ff68e42435310d47

Simplifies the usage of signals somewhat as can be seen in the modifications to the new unit tests in the second commit.

Made sure to test detail::atomic_shared_ptr by removing #ifdef wrappers.

TIL atomic_load()/atomic_store() are specialized for shared_ptr* up until C++26 - https://en.cppreference.com/cpp/memory/shared_ptr/atomic

sedited commented at 2:09 PM on October 21, 2026: contributor

@ryanofsky let me know if you want to pick up the nits here.