proxy-io.h: Add Connection disconnect and waitDrained methods - #335
proxy-io.h: Add Connection disconnect and waitDrained methods#335ryanofsky wants to merge 8 commits into
disconnect and waitDrained methods#335Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK |
|
CI seems upset? |
|
Updated 39ed2ca -> a40189f ( Added 1 commits a40189f -> 11929f1 ( Updated 11929f1 -> 901a090 ( |
enirox001
left a comment
enirox001
left a comment
There was a problem hiding this comment.
Code Review 901a090
Separating connection teardown from destruction and providing a server-call drain functioanlity is a good addition. The overall approach makes sense. I intend to review this more
I think the commit messages and code documentation are a bit too verbose. The explanations are nice to have, but it overexplains quite often, which ultimately makes it a bit harder to understand. Would suggest some revisions to the commit messages and the documentation to increase clarity
In commit 39cc757: ipc: add Connection::disconnect() separating teardown from destruction
I also think this is not exactly a behavior-neutral change; the commit message itself says Two details are new: as we now explicitly cancel m_on_disconnect handlers before severing the connection, and explicitly release m_thread_pool and m_thread_map during disconnect() rather than relying on member destruction.
The m_on_disconnect change is especially not something I would call behavior-neutral, as now we have to proactively cancel because Connection remains alive after the transport is severed and is no longer a consequence of destruction teardown. So even though the externally observable behaviour might seem unchanged, the lifetime and cancellation behaviour has changed, and I think that distinction matters
So the text saying
“This is a behavior-neutral refactor: the same steps run in the same order on destruction.”
is a bit misleading i think?
Also, in commit a40189f, there does not seem to be a clear commit title and description here; they are together
Left a few more suggestions and nits below
| // Disconnecting triggers I/O and tears down capnp state, so it must run on | ||
| // the event loop thread, like the destructor. | ||
| assert(std::this_thread::get_id() == m_loop->m_thread_id); | ||
| if (m_disconnected) return; |
There was a problem hiding this comment.
In commit 39cc757: ipc: add Connection::disconnect() separating teardown from destruction
disconnect() sets m_disconnected = true; later on we clean everything up, so I am unsure, but if there was a scenario where one of the cleanups threw, it would not complete the rest. This might not be a problem, but another call to disconnect() would be a no-op.
I do not think all the operations after this can cause this to throw and lead to this, but shutdownWrite() might if it throws an exception other than the ones mentioned.
A simple fix is to set the m_disconnected = true only after all teardown that must run has completed.
index 0aaa58a..8b9f458 100644
--- a/src/mp/proxy.cpp
+++ b/src/mp/proxy.cpp
@@ -124,7 +124,6 @@ void Connection::disconnect()
// the event loop thread, like the destructor.
assert(std::this_thread::get_id() == m_loop->m_thread_id);
if (m_disconnected) return;
- m_disconnected = true;
// Cancel pending onDisconnect handlers first. Severing the connection
// below completes m_network.onDisconnect() promises, and the registered
@@ -253,6 +252,8 @@ void Connection::disconnect()
// stream.
m_network.reset();
m_stream = nullptr;
+
+ m_disconnected = true;
}
void Connection::waitDrained()or a better solution that make sure the the cleanup happens even if shutdownWrite fails?
There was a problem hiding this comment.
re: #335 (comment)
I think I want to drop the m_disconnected variable and just treat m_network being nullopt the same as m_disconnected being true, which I think should be equivalent to your suggestions.
It's also true that cleanup functions throwing is not something that this library handles very well generally, and could handle better in many cases.
There was a problem hiding this comment.
re: #335 (comment)
I think I want to drop the
m_disconnectedvariable and just treatm_networkbeing nullopt the same asm_disconnectedbeing true, which I think should be equivalent to your suggestions.
I did drop this extra variable in latest push, but didn't look into the exception safety yet. As mentioned previously there are many other places in the library where unexpected exceptions from callbacks will cause problems. I do want revisit and see if there's an improvement that can be made here but would want to keep scope limited and not get into fixing preexisting problems because that could really increase the size of this change.
| // to the EventLoop TaskSet to avoid "Promise callback destroyed itself" | ||
| // error in the typical case where f deletes this Connection object. | ||
| m_on_disconnect.add(m_network.onDisconnect().then( | ||
| m_on_disconnect->add(m_network->onDisconnect().then( |
There was a problem hiding this comment.
In 39cc757: ipc: add Connection::disconnect() separating teardown from destruction
The listener now keeps a counter of the active connections added in 39a10ce. When it is full, it stops accepting new connections, and when a client disconnects, a callback decreases the counter, and the listener can start accepting again.
But when the server calls disconnect() it cancels that callback. The connection closes, but the counter does not change, so the listener might think it is full and never accept another connection
Added this change to so that the listener count can be updated for every disconnect, while automatic deletion happens only for remote disconnects
index 1f77b26..30627ec 100644
--- a/include/mp/proxy-io.h
+++ b/include/mp/proxy-io.h
@@ -1016,10 +1016,12 @@ void _Serve(EventLoop& loop, kj::Own<kj::AsyncIoStream>&& stream, InitImpl& init
auto it = loop.m_incoming_connections.begin();
MP_LOG(loop, Log::Info) << "IPC server: socket connected.";
if (loop.testing_hook_connected) loop.testing_hook_connected();
- it->onDisconnect([&loop, it, on_disconnect = std::forward<OnDisconnect>(on_disconnect)]() mutable {
+ it->addSyncCleanup([on_disconnect = std::forward<OnDisconnect>(on_disconnect)]() mutable {
+ on_disconnect();
+ });
+ it->onDisconnect([&loop, it]() mutable {
MP_LOG(loop, Log::Info) << "IPC server: socket disconnected.";
loop.m_incoming_connections.erase(it);
- on_disconnect();
if (loop.testing_hook_disconnected) loop.testing_hook_disconnected();
});
}This test could also be added to verify the above behaviour
index a9d4dca..240af3f 100644
--- a/test/mp/test/listen_tests.cpp
+++ b/test/mp/test/listen_tests.cpp
@@ -265,6 +265,29 @@ KJ_TEST("ListenConnections enforces a local connection limit")
KJ_EXPECT(client3->client->add(3, 4) == 7);
}
+KJ_TEST("ListenConnections resumes after a local disconnect")
+{
+ ListenSetup server(/*max_connections=*/1);
+
+ auto client1 = std::make_unique<ClientSetup>(server.listener.MakeConnectedSocket());
+ server.WaitForConnectedCount(1);
+ KJ_EXPECT(client1->client->add(1, 2) == 3);
+
+ auto client2 = std::make_unique<ClientSetup>(server.listener.MakeConnectedSocket());
+ (**server.m_loop_ref).sync([] {});
+ KJ_EXPECT(server.ConnectedCount() == 1);
+
+ EventLoop& loop{**server.m_loop_ref};
+ loop.sync([&] {
+ KJ_REQUIRE(loop.m_incoming_connections.size() == 1);
+ loop.m_incoming_connections.front().disconnect();
+ loop.m_incoming_connections.pop_front();
+ });
+
+ server.WaitForConnectedCount(2);
+ KJ_EXPECT(client2->client->add(2, 3) == 5);
+}
+
KJ_TEST("ListenConnections accepts multiple connections")
{
// With max-connections=2, two clients should be accepted and usable at theThere was a problem hiding this comment.
re: #335 (comment)
Good catch and nice test!
There was a problem hiding this comment.
re: #335 (comment)
Added this change to so that the listener count can be updated for every disconnect, while automatic deletion happens only for remote disconnects
Thanks for the bug report, and fix, and test. This is a separate, preexisting bug so I made a new PR #361 to address it. Your changes are in bb21177 there. This bug isn't a practical problem for bitcoin core because it does not disconnect IPC clients except when it is shutting down. But it could a problem for other IPC servers using this code. The problem is also not new to this PR. Even though this PR is adding a disconnect method which makes it possible to disconnect clients without deleting the Connection objects, it was always possible to disconnect clients by deleting the Connection objects.
| : m_loop(loop), m_stream(kj::mv(stream_)), | ||
| m_network(*m_stream, ::capnp::rpc::twoparty::Side::CLIENT, ::capnp::ReaderOptions()), | ||
| m_rpc_system(::capnp::makeRpcClient(m_network)) {} | ||
| m_network(std::in_place, *m_stream, ::capnp::rpc::twoparty::Side::CLIENT, ::capnp::ReaderOptions()), |
There was a problem hiding this comment.
In commit 39cc757: ipc: add Connection::disconnect() separating teardown from destruction
Connection can now remain alive after it has been disconnected, but the class does not define which methods are safe to call afterwards.
Previously, disconnection meant destroying the whole object, but now the connection object still exists. This is needed so callers can use methods such as waitDrained, but other methods still behave as if the connection is active.
Could some documentation, assertion, or runtime check be helpful for this?
There was a problem hiding this comment.
re: #335 (comment)
Connection can now remain alive after it has been disconnected, but the class does not define which methods are safe to call afterwards.
Added some documentation that after calling disconnect method methods that perform i/o won't work. In general, I would like to make methods safe to call and avoid having unnecessary restrictions.
| // Blocking the event loop thread here would deadlock: in-flight call | ||
| // bodies sync() back to the event loop to deliver their results, and | ||
| // server objects are destroyed on the event loop thread. | ||
| assert(std::this_thread::get_id() != m_loop->m_thread_id); |
There was a problem hiding this comment.
In 631d8d9: ipc: add Connection::waitDrained() to wait for in-flight server calls
The documentation for the waitDrained method in proxy-io.h says it is meant to be called after disconnect() call, but does nothing to enforce it, i think we can assert that the disconnect method has been called before calling waitDrained as such
index 0aaa58a..6a318b0 100644
--- a/src/mp/proxy.cpp
+++ b/src/mp/proxy.cpp
@@ -261,6 +261,7 @@ void Connection::waitDrained()
// bodies sync() back to the event loop to deliver their results, and
// server objects are destroyed on the event loop thread.
assert(std::this_thread::get_id() != m_loop->m_thread_id);
+ assert(m_disconnected);
m_server_objects->wait();
}There was a problem hiding this comment.
re: #335 (comment)
i think we can assert that the disconnect method has been called before calling waitDrained as such
Was there a specific scenario that made this assert seem useful? It should be fine to call waitDrained regardless of whether the disconnect method was called. It would seem useful to do that if you want to detect when there's a disconnect and server objects are no longer in use, and don't care whether the disconnect was initiated locally or remotely.
| // disconnect error), but the body is still blocked on the worker thread, | ||
| // so its server object must still be alive. | ||
| foo->m_context.loop->sync([&] { connection->disconnect(); }); | ||
| KJ_EXPECT(connection->pendingServerObjects() == 1); |
There was a problem hiding this comment.
In commit 092d1db: test: cover draining in-flight server call after disconnect
I think using an exact count of one is a bit brittle; the test only needs to establish that something remains in flight. This would be less implementation-specific
index 5bccb86..da47fde 100644
--- a/test/mp/test/test.cpp
+++ b/test/mp/test/test.cpp
@@ -463,13 +463,13 @@ KJ_TEST("Waiting for in-flight server call to finish after disconnect")
// The FooInterface server object is the connection's only counted server
// object, and its call body is executing.
- KJ_EXPECT(connection->pendingServerObjects() == 1);
+ KJ_EXPECT(connection->pendingServerObjects() > 0);
// Disconnect. This cancels the call's promise (the client above sees the
// disconnect error), but the body is still blocked on the worker thread,
// so its server object must still be alive.
foo->m_context.loop->sync([&] { connection->disconnect(); });
- KJ_EXPECT(connection->pendingServerObjects() == 1);
+ KJ_EXPECT(connection->pendingServerObjects() > 0);
// A drain must block while the body runs and return only once it
// finishes, which is what Ipc::disconnectIncoming relies on duringThere was a problem hiding this comment.
re: #335 (comment)
I think using an exact count of one is a bit brittle; the test only needs to establish that something remains in flight. This would be less implementation-specific
It is true test could be a little more brittle, but I think checking actual counts makes the test easier to understand and makes the code match the comments. But I would agree if there was a change that did cause these checks to break, that would be evidence these are too brittle, and would be good to make checks less strict at that point.
| // concurrently remove entries when connections are broken (see SetThread | ||
| // cleanup function), then destroy the removed ProxyClient<Thread> with the | ||
| // mutex released, since its destructor needs to lock EventLoop::m_mutex | ||
| // and Waiter::m_mutex must not be held when EventLoop::m_mutex is |
There was a problem hiding this comment.
In commit 901a090: Fix thread map teardown race causing use-after-free on disconnect
The Waiter documentation says
//! This mutex can be held at the same time as
//! EventLoop::m_mutex as long as Waiter::mutex is locked first and
//! EventLoop::m_mutex is locked second.
But the new commit says
//! Waiter::m_mutex must not be held when EventLoop::m_mutex is
//! acquired
It also says releasing the waiter mutex avoids locking the Waiter mutex before the EventLoop mutex, these rules cannot both be correct.
I think an actual order should be identified and updated here
There was a problem hiding this comment.
re: #335 (comment)
I think an actual order should be identified and updated here
Nice catch. This is actually not the first comment that got the order backwards so I added a new commit to base PR #361 to improve all the documentation about lock order. (It's part of #361 not this PR because I also moved the "Fix thread map teardown race causing" commit you referenced to #361.)
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Thanks for the review! Great catches and suggestions. Just left some quick feedback below to make sure I didn't miss anything
| // Disconnecting triggers I/O and tears down capnp state, so it must run on | ||
| // the event loop thread, like the destructor. | ||
| assert(std::this_thread::get_id() == m_loop->m_thread_id); | ||
| if (m_disconnected) return; |
There was a problem hiding this comment.
re: #335 (comment)
I think I want to drop the m_disconnected variable and just treat m_network being nullopt the same as m_disconnected being true, which I think should be equivalent to your suggestions.
It's also true that cleanup functions throwing is not something that this library handles very well generally, and could handle better in many cases.
| // to the EventLoop TaskSet to avoid "Promise callback destroyed itself" | ||
| // error in the typical case where f deletes this Connection object. | ||
| m_on_disconnect.add(m_network.onDisconnect().then( | ||
| m_on_disconnect->add(m_network->onDisconnect().then( |
There was a problem hiding this comment.
re: #335 (comment)
Good catch and nice test!
|
Code review 901a090 I agree with @enirox001 that some comments and commit messages are quite confusing. Some are written like a story, e.g., "Previously..." clauses that add little value to the code. I had to ignore them because reading the code itself was simpler for me to understand the changes. Planning to review again once there are more updates. |
…sier to understand. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
901a090 to
33ab215
Compare
|
Thanks for the reviews! I implemented fixes for the two preexisting bugs that were pointed out here in a new PR #361, that this PR is now based on so it would makes sense to review that PR first. I've partially addressed some other comments here as well but am still working on things. Rebased 901a090 -> 33ab215 ( |
Waiter::m_mutex must not be locked before EventLoop::m_mutex. That is the reverse of the documented and actual lock order (Waiter::m_mutex first, as ~ProxyServer<Thread> does). The constraint it was reaching for is the EventLoop blocking rule now documented on Waiter::m_mutex. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YKQfSnMnUzqyDxKp7GpFam
Currently, a ListenConnections listener that reaches its max-connection limit stops accepting new connections permanently if one of its connections is closed locally instead of by a remote disconnect. Closing a connection locally (e.g. erasing it from m_incoming_connections) leaves the listener's active-connection count stuck at the limit, so it never resumes accepting. This happens because the count is decremented by a callback which only fires on a remote disconnects, not local disconnects. Fix by moving the decrement to callback which fires on both local and remote disconnects. Add a regression test that closes a connection locally and checks the listener resumes accepting; it fails before this change (the listener never accepts the waiting client) and passes after. Co-Authored-By: Enoch Azariah <enirox001@gmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fix a use-after-free, possible since the destroy_connection option was added in 2019 (c685fa9): a Connection's disconnect handler could run after the Connection had already been destroyed, deleting it a second time and crashing. Reported by enirox001 in bitcoin-core#335 (comment) Give each Connection a shared_ptr "alive" token that disconnect handlers hold a weak_ptr to and check before running, so a handler is skipped once its Connection is gone. Having this check also enables the simplifications described below. Previously each Connection kept its disconnect handlers in its own kj::TaskSet, and when the network disconnected it moved a handler onto the shared event loop TaskSet with kj::evalLater. Destroying the Connection destroyed that per-connection TaskSet, canceling a still-pending handler -- but a handler already moved onto the shared TaskSet was no longer canceled and could run after the Connection was gone. (The evalLater step existed only to avoid a "promise callback destroyed itself" error when a handler deletes its own Connection, which the per-connection TaskSet made possible.) With the token doing the cancellation, neither the per-connection TaskSet nor the evalLater step is needed, and both are removed. Co-Authored-By: Enoch Azariah <enirox001@gmail.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
connection being destroyed on the event loop thread, which could destroy the same ProxyClient<Thread> object twice. ~ThreadContext destroyed the thread-local request_threads/callback_threads maps with no locking while the SetThread cleanup callback run by ~Connection erased entries from the same maps. When both ran at once, each side destroyed the entry's ProxyClient<Thread>, and ~Connection then ran the ProxyClientBase disconnect callback on the freed map node (heap-use-after-free, then a glibc "double free or corruption" abort). Fix by making map entry removal decide which side destroys an entry: ~ThreadContext and the SetThread callback each remove entries under Waiter::m_mutex before destroying them, and a side that finds an entry already gone leaves it to the other. See the code comments for why the entries are destroyed with the mutex released. Add a regression test, "Thread exiting while its connection is destroyed", which uses a new testing_hook_thread_client_destroy hook to interleave the two sides deterministically and fails on every run without the fix. The race is long-standing and reachable on master via connections created by ConnectStream, whose onDisconnect handler deletes the client Connection on the event loop thread when the peer disconnects. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnBLP1xuPf4fLpnQto8mEX Claude-Session: https://claude.ai/code/session_01YKQfSnMnUzqyDxKp7GpFam
…uction Split connection teardown out of ~Connection into an idempotent disconnect() method, with the destructor delegating to it. For existing callers, this is a behavior-neutral refactor: the same steps run in the same order on destruction. Having a separate disconnect() method allows severing a connection while keeping the Connection object alive, which the next commits use to let shutdown code wait for in-flight server call bodies to finish after a disconnect (bitcoin/bitcoin#35845). Two details are new in the disconnect() method which were not present in the destructor method: - disconnect() expires the m_alive token explicitly, where previously it was expired implicitly by member destruction. This keeps onRemoteDisconnect able to distinguish a local disconnect from a remote one when a connection is severed without destroying the object (see the disconnect() code comment). - disconnect() explicitly releases m_thread_pool and m_thread_map so worker thread teardown happens at disconnect time whether or not the object is destroyed right away. Previously this happened implicitly during member destruction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…calls Add a per-connection ServerObjectTracker counting live ProxyServer objects, incremented in the ProxyServerBase constructor and decremented in its destructor, with Connection::waitDrained() blocking until the count reaches zero and Connection::pendingServerObjects() exposing it for logging. Disconnecting a connection cancels the KJ promise of an in-flight call, but a C++ server method body already dispatched to a worker thread runs to completion. Counting live server objects turns Cap'n Proto's object lifetime rules into a usable quiescence signal: a ProxyServer object is not destroyed until its outstanding calls finish (the target capability is kept alive for the duration of a call and pinned by post()/PassField via thisCap()), so after disconnect() the count drains to zero exactly when no server call body is still executing. Waiting for that lets shutdown code avoid freeing application state that a still-running call body dereferences (bitcoin/bitcoin#35845). The tracker is held via shared_ptr by the Connection and by every ProxyServer object because objects kept alive by in-flight calls can outlive the Connection on some teardown paths (see ~ProxyServerBase), and their destructors must decrement state that is still valid. It must be declared before m_rpc_system, whose construction creates the bootstrap server object that registers itself with the tracker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add a deterministic mptest regression test for bitcoin/bitcoin#35845: hold a server method body in flight on a worker thread, call Connection::disconnect(), and assert that Connection::waitDrained() blocks until the body finishes and its server object is destroyed. Also covers destroying an already-disconnected connection (~Connection noticing disconnect() has run). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Enoch Azariah <enirox001@gmail.com>
Fix a use-after-free, possible since the destroy_connection option was added in 2019 (c685fa9): a Connection's disconnect handler could run after the Connection had already been destroyed, deleting it a second time and crashing. Reported by enirox001 in bitcoin-core#335 (comment) Give each Connection a shared_ptr "alive" token that disconnect handlers hold a weak_ptr to and check before running, so a handler is skipped once its Connection is gone. Having this check also enables the simplifications described below. Previously each Connection kept its disconnect handlers in its own kj::TaskSet, and when the network disconnected it moved a handler onto the shared event loop TaskSet with kj::evalLater. Destroying the Connection destroyed that per-connection TaskSet, canceling a still-pending handler -- but a handler already moved onto the shared TaskSet was no longer canceled and could run after the Connection was gone. (The evalLater step existed only to avoid a "promise callback destroyed itself" error when a handler deletes its own Connection, which the per-connection TaskSet made possible.) With the token doing the cancellation, neither the per-connection TaskSet nor the evalLater step is needed, and both are removed. Co-Authored-By: Enoch Azariah <enirox001@gmail.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
33ab215 to
40cfb93
Compare
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Updated 33ab215 -> 40cfb93 (pr/keepconn.5 -> pr/keepconn.6, compare) improving comments and naming and moving more changes to base PR #361 to simplify commits here.
re: #335 (review)
I also think this is not exactly a behavior-neutral change
Thanks, clarified commit message to say this is a behavior-neutral change for existing callers not calling the new methods. For callers that do specifically call the new disconnect() method, there are some differences from destroying the Connection object.
| // disconnect error), but the body is still blocked on the worker thread, | ||
| // so its server object must still be alive. | ||
| foo->m_context.loop->sync([&] { connection->disconnect(); }); | ||
| KJ_EXPECT(connection->pendingServerObjects() == 1); |
There was a problem hiding this comment.
re: #335 (comment)
I think using an exact count of one is a bit brittle; the test only needs to establish that something remains in flight. This would be less implementation-specific
It is true test could be a little more brittle, but I think checking actual counts makes the test easier to understand and makes the code match the comments. But I would agree if there was a change that did cause these checks to break, that would be evidence these are too brittle, and would be good to make checks less strict at that point.
| // concurrently remove entries when connections are broken (see SetThread | ||
| // cleanup function), then destroy the removed ProxyClient<Thread> with the | ||
| // mutex released, since its destructor needs to lock EventLoop::m_mutex | ||
| // and Waiter::m_mutex must not be held when EventLoop::m_mutex is |
There was a problem hiding this comment.
re: #335 (comment)
I think an actual order should be identified and updated here
Nice catch. This is actually not the first comment that got the order backwards so I added a new commit to base PR #361 to improve all the documentation about lock order. (It's part of #361 not this PR because I also moved the "Fix thread map teardown race causing" commit you referenced to #361.)
4 participants
Note: This is based on #361. Initial commits should be reviewed in that PR.
Add
ConnectionclassdisconnectandwaitDrainedmethods to provide more flexibility when forcibly disconnecting from remote clients or servers.Without these methods, the only way to forcibly close IPC connections is to delete
Connectionobjects. This works but is not ideal because once aConnectionobject is gone, it is difficult to track state still associated with the connection, particularly:ProxyServerobjects that may still be alive because they are executing asynchronous requests made before the disconnect. Without a way to track these objects, there is no generic way to wait for requests to finish existing after disconnecting. So individual IPC interfaces like the Bitcoin mining interface would need to implement custom synchronization to avoid race conditions during shutdown. Followup PR ipc: make ipc::disconnectIncoming wait for in-progress calls to complete bitcoin/bitcoin#35932 builds on this PR, calling the newwaitDrainedmethod introduced here to avoid IPC mining crashes on Bitcoin core shutdown without needing to change the mining code. A unit test is added here simulating these mining crashes.ProxyClientobjects that contain pointers toConnectionobjects. CurrentlyProxyClientobject need to register cleanup handlers withConnectionobjects to deal with Connections being deleted, which consumes memory and complicatesProxyClientshutdown logic. After this change, a followup PR will drop the cleanup handlers soConnectionobjects no longer need to track lists ofProxyClientobjects associated with them. This is implemented in proxy-io: Reference-count Connection objects #336.