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 ConflictsNo conflicts as of last run. LLM Linter (✨ experimental)Possible typos and grammar issues:
2026-09-04 20:26:00 |
|
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
Before this PR, when a remote side disconnected, libmultiprocess had callbacks that would eventually remove the Connection. It does not necessarily call the remove operation immediately; it can schedule it into another task set. The new disconnect() wants different behaviour. such that it will disconnect and then call waitDrained later on. So it tries to reset the m_on_disconnect callbacks.
But if the callback has already progressed one step further before reset happens, this violates the goal of this new system.
In aa49a11 this is made to use a weak_ptr, but I wonder if we should move those changes to this pr instead? Or rather, a small cancellation guard could be added to this PR such that it keeps the existing changes focused while preventing the potential regression.
A minimal change adding a weak cancelation token that has moved into the event loop queue.
index 1f77b26..d817eb6 100644
--- a/include/mp/proxy-io.h
+++ b/include/mp/proxy-io.h
@@ -576,8 +576,18 @@ public:
// handler fires, do not call the function f right away, instead add it
// to the EventLoop TaskSet to avoid "Promise callback destroyed itself"
// error in the typical case where f deletes this Connection object.
+ const std::weak_ptr<void> guard{m_on_disconnect_guard};
m_on_disconnect->add(m_network->onDisconnect().then(
- [f = std::forward<F>(f), this]() mutable { m_loop->m_task_set->add(kj::evalLater(kj::mv(f))); }));
+ [f = std::forward<F>(f), guard, this]() mutable {
+ m_loop->m_task_set->add(kj::evalLater(
+ [f = kj::mv(f), guard]() mutable {
+ // The connection-owned TaskSet may have already handed
+ // this callback to the event-loop TaskSet by the time
+ // disconnect() cancels it. Only run it if the
+ // connection has not been disconnected in between.
+ if (guard.lock()) f();
+ }));
+ }));
}
EventLoopRef m_loop;
@@ -587,6 +597,10 @@ public:
//! disconnections, if the connection is closed locally first by deleting
//! this Connection object.
std::optional<kj::TaskSet> m_on_disconnect{std::in_place, m_error_handler};
+ //! Lifetime token checked by onDisconnect handlers after they are handed
+ //! off to the EventLoop TaskSet. Reset by disconnect() so a handler already
+ //! queued there cannot run after local teardown.
+ std::shared_ptr<void> m_on_disconnect_guard{std::make_shared<char>()};
//! Wrapped in std::optional so disconnect() can destroy it (and m_stream
//! below) to sever the transport while this object stays alive. Closing
//! the stream is what makes the peer observe the disconnect: it reads EOF
diff --git a/src/mp/proxy.cpp b/src/mp/proxy.cpp
index 0aaa58a..06063f8 100644
--- a/src/mp/proxy.cpp
+++ b/src/mp/proxy.cpp
@@ -133,6 +133,7 @@ void Connection::disconnect()
// harmful when disconnect() is called separately by code that keeps using
// the object afterwards (e.g. code waiting for in-flight calls to finish
// before destroying it).
+ m_on_disconnect_guard.reset();
m_on_disconnect.reset();
// Try to cancel any calls that may be executing.This change closes the gap where resetting m_on_disconnect was too late because the callback had already moved into the EventLoop task set
There was a problem hiding this comment.
re: #335 (comment)
Hmm, this is an interesting finding. But if there is a bug here, it seems like a pre-existing one, not something caused by this change or made worse by it.
You're saying if a remote disconnect happens first, and the m_network->onDisconnect() callback executes, but the kj::evalLater callback inside it does not execute yet, and if within that interval, the local process decided to delete the connection, then the connection could be deleted twice.
This does seem like it might be possible, and I'd want to look into it a little more and write a test. I'd still be inclined to save a fix for a different PR, and I believe as you pointed out #336 might fix this.
There was a problem hiding this comment.
This is not exactly the point I intended to pass across; it was more
- remote callback queued
- local code calls
disconnect(), intending to keep the connection alive - local code retains the connection pointer for
waitDrained - queued callback erases and destroys the
Connection - shutdown code uses the dangling connection pointer
It is not a
- local code deletes the connections
- queued callback deletes it again
But I think it could be possible for the connection object to be deleted twice, once by the object and another time by a queued callback (which is similar to the original concern i had) and yes, this would be a pre-existing issue
I added a test to verify this (to an extent). First of all, I added a hook to be called before an onDisconnect callback is queued on the event loop
index 1f77b26..100bd10 100644
--- a/include/mp/proxy-io.h
+++ b/include/mp/proxy-io.h
@@ -378,6 +378,9 @@ public:
//! Hook called on the event loop thread when a client has disconnected.
std::function<void()> testing_hook_disconnected;
+
+ //! Hook called before an onDisconnect callback is queued on the event loop.
+ std::function<void()> testing_hook_before_on_disconnect_queued;
};
//! Single element task queue used to handle recursive capnp calls. (If the
@@ -577,7 +580,12 @@ public:
// 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(
- [f = std::forward<F>(f), this]() mutable { m_loop->m_task_set->add(kj::evalLater(kj::mv(f))); }));
+ [f = std::forward<F>(f), this]() mutable {
+ if (m_loop->testing_hook_before_on_disconnect_queued) {
+ m_loop->testing_hook_before_on_disconnect_queued();
+ }
+ m_loop->m_task_set->add(kj::evalLater(kj::mv(f)));
+ }));
}
EventLoopRef m_loop;and then wrote a test that cancels an already queued onDisconnect callback
index 5bccb86..4fd906c 100644
--- a/test/mp/test/test.cpp
+++ b/test/mp/test/test.cpp
@@ -291,6 +291,42 @@ KJ_TEST("Calling IPC method after server connection is closed")
EXPECT_EXCEPTION(foo->add(1, 2), "IPC client method call interrupted by disconnect.");
}
+KJ_TEST("Destroying a connection cancels an already queued onDisconnect callback")
+{
+ std::promise<bool> result;
+ std::thread loop_thread{[&] {
+ EventLoop loop("mptest", [](mp::LogMessage) {});
+ auto pipe = loop.m_io_context.provider->newTwoWayPipe();
+ auto server_connection =
+ std::make_unique<Connection>(loop, kj::mv(pipe.ends[0]), [&](Connection& connection) {
+ return capnp::Capability::Client(kj::heap<ProxyServer<messages::FooInterface>>(
+ std::make_shared<FooImplementation>(), connection));
+ });
+ auto client_connection = std::make_unique<Connection>(loop, kj::mv(pipe.ends[1]));
+ auto client = client_connection->m_rpc_system->bootstrap(ServerVatId().vat_id).castAs<messages::FooInterface>();
+ bool callback_ran{false};
+
+ server_connection->onDisconnect([&] { callback_ran = true; });
+ loop.testing_hook_before_on_disconnect_queued = [&] {
+ loop.m_task_set->add(kj::evalLater([&] {
+ server_connection.reset();
+ loop.m_task_set->add(kj::evalLater([&] {
+ client = nullptr;
+ client_connection.reset();
+ result.set_value(callback_ran);
+ }));
+ }));
+ };
+
+ loop.m_task_set->add(kj::evalLater([&] { client_connection->disconnect(); }));
+ loop.loop();
+ }};
+
+ const bool callback_ran{result.get_future().get()};
+ loop_thread.join();
+ KJ_EXPECT(!callback_ran);
+}
+
KJ_TEST("Calling IPC method and disconnecting during the call")
{
TestSetup setup{/*client_owns_connection=*/false}This test fails
This shows that the onDisconnect callback can execute after its owning Connection has already been destroyed, and if this happens to try to delete the Connection object, it could lead to undefined behavior
There was a problem hiding this comment.
re: #335 (comment)
- shutdown code uses the dangling connection pointer
Thanks yes that makes sense. I was assuming the only thing shutdown code would be doing with the pointer would be deleting it. But of course if it did something else with the pointer (like call waitDrained) the symptom of the bug would be a use-after-free and not a double delete.
Either way, this is a preexisting bug, so I adopted your test and fix and used them in #361 commit bc98767.
| // 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.
| //! dereferencing application state that is about to be freed) after | ||
| //! incoming connections are disconnected. See Ipc::disconnectIncoming and | ||
| //! https://github.com/bitcoin/bitcoin/issues/35845. | ||
| void waitDrained(); |
There was a problem hiding this comment.
In 631d8d9: ipc: add Connection::waitDrained() to wait for in-flight server calls
nit:
The name waitDrained could be clearer if it is called waitServerCallsDrained? or at least document its exact scope a bit more clearly
There was a problem hiding this comment.
re: #335 (comment)
The name waitDrained could be clearer if it is called waitServerCallsDrained? or at least document its exact scope a bit more clearly
This is a difficult method to name and I think it might be better to focus on improving documentation if something is unclear. The specific problem with waitServerCallsDrained is this doesn't just wait for calls to finish it also waits for objects to be released.
Fundamentally this method is meant to be useful for waiting until it is safe to free resources associated with the connection, and I think it makes sense to name it after its purpose instead after how it happens to be implemented at the moment. It could easily be implemented other ways such as by counting calls directly instead of on relying on PassField for mp.Context parameters using thisCap.
I did make a number of documentation updates here and the main place semantics of object counting are explained is the ServerObjectTracker documentation comment.
| // 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 during| }); | ||
|
|
||
| // The body is still blocked, so waitDrained() must not have returned. | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(20)); |
There was a problem hiding this comment.
In commit 092d1db: test: cover draining in-flight server call after disconnect
The 20 ms check is a bit too scheduler dependent, if the drain thread has not been scheduled during that interval, drained remains false even if waitDrained() is broken and would return immediately causing false pass.
We could add a hook here that runs only when ServerObject::wait() sees a non zero count and is about to wait
index 1f77b26..ab506b6 100644
--- a/include/mp/proxy-io.h
+++ b/include/mp/proxy-io.h
@@ -494,12 +494,14 @@ struct ServerObjectTracker
void wait()
{
Lock lock(m_mutex);
+ if (m_count != 0 && testing_hook_wait) testing_hook_wait();
m_cv.wait(lock.m_lock, [this]() MP_REQUIRES(m_mutex) { return m_count == 0; });
}
mutable Mutex m_mutex;
std::condition_variable m_cv;
size_t m_count MP_GUARDED_BY(m_mutex){0};
+ std::function<void()> testing_hook_wait;
};
//! Object holding network & rpc state associated with either an incoming server
diff --git a/test/mp/test/test.cpp b/test/mp/test/test.cpp
index 5bccb86..eec52f6 100644
--- a/test/mp/test/test.cpp
+++ b/test/mp/test/test.cpp
@@ -474,13 +474,17 @@ KJ_TEST("Waiting for in-flight server call to finish after disconnect")
// A drain must block while the body runs and return only once it
// finishes, which is what Ipc::disconnectIncoming relies on during
// shutdown.
+ std::promise<void> drain_waiting;
+ connection->m_server_objects->testing_hook_wait = [&] { drain_waiting.set_value(); };
std::atomic<bool> drained{false};
std::thread drain_thread([&] {
connection->waitDrained();
drained = true;
});
- // The body is still blocked, so waitDrained() must not have returned.
+ // Wait until waitDrained() has observed the live server object and is
+ // about to block, then verify it does not return while the body is blocked.
+ drain_waiting.get_future().get();
std::this_thread::sleep_for(std::chrono::milliseconds(20));
KJ_EXPECT(!drained);The test will then wait for that hook before starting the 20ms check. This ensures the drain thread has entered wait and observed pending work. This substantially reduces the possibility of a false positive
| // 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
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)
Hmm, this is an interesting finding. But if there is a bug here, it seems like a pre-existing one, not something caused by this change or made worse by it.
You're saying if a remote disconnect happens first, and the m_network->onDisconnect() callback executes, but the kj::evalLater callback inside it does not execute yet, and if within that interval, the local process decided to delete the connection, then the connection could be deleted twice.
This does seem like it might be possible, and I'd want to look into it a little more and write a test. I'd still be inclined to save a fix for a different PR, and I believe as you pointed out #336 might fix this.
| // 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. |
Two Connection callback-registration methods had names that did not match when they actually run: - onDisconnect() only ran its handler on a *remote* disconnect (it is canceled when the connection is disconnected locally), so rename it to onRemoteDisconnect(), and rename its backing TaskSet m_on_disconnect to m_on_remote_disconnect. - addSyncCleanup()/removeSyncCleanup() registered a function that runs on *any* disconnect (it is invoked from the connection teardown path), so rename them to onDisconnect()/cancelOnDisconnect(). Pure rename, no behavior change. The honest names make the following commits easier to follow: the next commit moves a listener bookkeeping callback from onRemoteDisconnect() to onDisconnect() so it runs on local disconnects too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A ListenConnections listener that reached its max-connection limit would stop accepting new connections permanently if one of its connections was closed locally instead of by a remote disconnect. Closing a connection locally (e.g. erasing it from m_incoming_connections) left the listener's active-connection count stuck at the limit, so it never resumed accepting. The count is decremented by the on_disconnect callback, which ran from the _Serve onRemoteDisconnect handler. That handler only fires on a remote disconnect and is canceled when a connection is closed locally, so the decrement was skipped on local closes. Register on_disconnect with onDisconnect() instead, so it runs on the connection teardown path for both remote and local disconnects, and keep only the list erase on onRemoteDisconnect(). 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> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uction Split connection teardown out of ~Connection into an idempotent disconnect() method, with the destructor delegating to it. 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: - 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 accessors to avoid libmultiprocess applications needing to create Connection objects directly or access their internals. It adds an EventLoop::incomingConnections method and a ServeStream overload that accepts a shared interface pointer instead a reference. This is just a refactoring that does not change behavior. It allows Bitcoin Core code to be simplified and to avoiding needing to change again with upcoming PRs such as bitcoin-core#336 which the change the way Connection objects work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fix a race between a thread exiting after making IPC calls and a
connection being destroyed by its onDisconnect handler on the event loop
thread. The race was between ~ThreadContext destroying the thread-local
request_threads/callback_threads maps with no locking, and the SetThread
cleanup function (run by Connection::disconnect) erasing entries from
those maps on the event loop thread. When the two ran concurrently, both
could destroy the same ProxyClient<Thread> object: the SetThread cleanup
reset m_disconnect_cb just before ~ProxyClient<Thread> checked it
unsynchronized, so the exiting thread proceeded to destroy the object
while the event loop's map erase destroyed it too. The doubled
destruction consumed m_context.cleanup_fns on one thread, so the other
never unregistered the ProxyClientBase disconnect callback, and
Connection::disconnect then invoked that callback on the freed map node
(heap-use-after-free reading m_client, followed by a double free of the
node reported by glibc as "double free or corruption").
Fix by making map entry removal the synchronization point deciding which
side destroys each ProxyClient<Thread>:
- Add an explicit ~ThreadContext that removes map entries one at a time
under Waiter::m_mutex and destroys each removed node after releasing
the mutex (so ~ProxyClient<Thread> can lock EventLoop::m_mutex without
violating lock order), instead of destroying the maps unlocked.
- Change the SetThread cleanup function to look its entry up by
connection key under Waiter::m_mutex instead of dereferencing the
captured map iterator, extract it, and destroy the node outside the
lock, following the same pattern PassField already uses for mp.Context
arguments. If the entry is gone, the owning thread extracted it first
and is responsible for destroying it.
- Guard the removeSyncCleanup call in ~ProxyClient<Thread> with a
m_context.connection check, because when the entry was extracted by
~ThreadContext first, a concurrent disconnect still runs both the
SetThread cleanup (a no-op now) and the ProxyClientBase disconnect
callback, leaving m_disconnect_cb set but pointing at a spliced-out
list iterator that must not be passed to removeSyncCleanup. The
disconnect callback nulls m_context.connection, and posted functions
cannot interleave with Connection::disconnect on the event loop
thread, so a null connection reliably indicates this case.
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 while an
exiting thread may be running ~ThreadContext. It was exposed by the
"Waiting for in-flight server call to finish after disconnect" test
because commit bb47369f202b62b8b64f5a52984ff2c40d64ecdd ("Fix error
handling when creating clients") extended the delete-on-disconnect
handler to every ProxyClient created with destroy_connection=true,
including the test setup's directly-created client connection: the
server-side disconnect in the test then deleted the client Connection on
the event loop thread exactly while the test's call thread was exiting.
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>
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 ( |
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.