refactor: EventLoop locking cleanups + client disconnect exception

[ryanofsky]

This PR was originally written as a code cleanup to follow up on review comments in earlier PRs. Several other commits were added afterward building on the earlier changes, and necessary for the bugfixes in bitcoin/bitcoin#32345, which resolves issues #123, #176 and #182.

Summary of changes:

• The commit "Improve IPC client disconnected exceptions" is the only external change, and lets clients detect when they are trying to call a remote method after a connection has been closed. This change is used by
  bitcoin/bitcoin#32345 commit "ipc: Handle bitcoin-wallet disconnection" to fix #123.
• The commits "Prevent EventLoop async cleanup thread early exit during shutdown" and "Prevent IPC server crash if disconnected during IPC call" fix issues reported in #182.
• New tests are added in other commits to verify these fixes, covering different kinds of disconnections.
• The change to detect disconnects also relies on an earlier "Add ProxyContext EventLoop* member" commit.
• An "Add EventLoopRef RAII class" commit simplifies a recent bugfix in #159 and also relies on the earlier "EventLoop* member" commit.
• The "Remove DestructorCatcher and AsyncCallable" commit follows up on #144 (comment).
• The "Add clang thread safety annotations" commit follows up on #129 (comment).

[ryanofsky]

Because this change removes EventLoop addClient and removeClient methods it will require an update to Bitcoin core if it is merged.

--- a/src/ipc/capnp/protocol.cpp
+++ b/src/ipc/capnp/protocol.cpp
@@ -41,10 +41,7 @@ class CapnpProtocol : public Protocol
 public:
     ~CapnpProtocol() noexcept(true)
     {
-        if (m_loop) {
-            std::unique_lock<std::mutex> lock(m_loop->m_mutex);
-            m_loop->removeClient(lock);
-        }
+        m_loop_ref.reset();
         if (m_loop_thread.joinable()) m_loop_thread.join();
         assert(!m_loop);
     };
@@ -83,10 +80,7 @@ public:
         m_loop_thread = std::thread([&] {
             util::ThreadRename("capnp-loop");
             m_loop.emplace(exe_name, &IpcLogFn, &m_context);
-            {
-                std::unique_lock<std::mutex> lock(m_loop->m_mutex);
-                m_loop->addClient(lock);
-            }
+            m_loop_ref.emplace(*m_loop);
             promise.set_value();
             m_loop->loop();
             m_loop.reset();
@@ -96,6 +90,7 @@ public:
     Context m_context;
     std::thread m_loop_thread;
     std::optional<mp::EventLoop> m_loop;
+    std::optional<mp::EventLoopRef> m_loop_ref;
 };
 } // namespace

[ryanofsky]

Updated ce4814f -> b47ea9f (pr/eventlock.1 -> pr/eventlock.2, compare), making test and comment fixes, splitting commits, and adding connection disconnects tests and exceptions to help resolve #123

[ryanofsky]

Rebased b47ea9f -> f15ef6c (pr/eventlock.2 -> pr/eventlock.3, compare) so this can be merged cleanly in bitcoin PR bitcoin/bitcoin#32345. Also dropped DisconnectError exception type so original ipc::Exception can be used instead.

[ryanofsky]

Rebased f15ef6c -> 2214358 (pr/eventlock.3 -> pr/eventlock.4, compare) to fix conflict with #165

[ryanofsky]

Rebased 2214358 -> 6715a96 (pr/eventlock.4 -> pr/eventlock.5, compare) to fix conflicts with #172

[ryanofsky]

Rewrote the PR description since this is now a dependency of bitcoin/bitcoin#32345 and needed to fix bugs #123 and #176

[ryanofsky]

Rebased 0b45cb5 -> 4c33734 (pr/eventlock.6 -> pr/eventlock.7, compare) due to conflict with #181, also adding 3 commits to fix #182, and making minor cleanups to earlier commits (mainly comments)
Updated 4c33734 -> 2dcfb46 (pr/eventlock.7 -> pr/eventlock.8, compare) to fix spurious thread safety warning https://cirrus-ci.com/task/4956347122319360 and spelling errors
Updated 2dcfb46 -> 84cf56a (pr/eventlock.8 -> pr/eventlock.9, compare) to fix spurious thread safety warning https://cirrus-ci.com/task/5066891695226880

[TheCharlatan]

As far as I can tell it is correct to not copy over the lock from the moved object here as long as the moved object has no lock. Should that be asserted?

[ryanofsky]

re: #160 (comment)

In commit "proxy-io.h: Add EventLoopRef RAII class handle addClient/removeClient refcounting" (9aaeec3)

As far as I can tell it is correct to not copy over the lock from the moved object here as long as the moved object has no lock. Should that be asserted?

It's kind of a corner case, and I think either behavior could be useful here. The point of this constructor is to be able to efficiently move a reference count from one owner to another would pointlessly incrementing & decrement the count when creating one reference and dropping the other.

I feel like it probably would not be useful to copy the m_lock pointer when doing that because practical effect of copying would be to make the new EventLoopRef object destructor skip locking when it is destroyed (it would assert m_lock is locked instead).

On the other hand the default move-constructor would copy the m_lock value so maybe it is less surprising to follow the default behavior. And if the caller really wanted m_lock to be null after moving they could set it to null themselves.

So this is a good catch and I think I'd now lean towards adding m_lock{other.m_lock} but I don't think the difference should matter in practice.

More broadly I was also thinking about whether I could split EventLoopRef into a superclass without an m_lock member and a subclass with one to be able to get rid of the relock option added in ea38392 which requires turning off thread safety analysis in the reset function. At the moment this class seems good at preventing reference counting bugs because it guarantees reference counts will always be decremented if they are incremented, but it doesn't have very easy to understand locking semantics so that's probably an area for improvement.

[TheCharlatan]

Is there a chance of encountering a race condition here between checking m_async_fns and calling ->empty()?

[ryanofsky]

re: #160 (comment)

In commit "Prevent EventLoop async cleanup thread early exit during shutdown" (ea38392)

Is there a chance of encountering a race condition here between checking m_async_fns and calling ->empty()?

There shouldn't be just because m_async_fns is guarded by m_mutex which is locked above.

[TheCharlatan]

The previous test exercises a server disconnect. Would it be useful to either add another case, or expand it to test the server going away during a client call? I tried just setting

    setup.server->m_impl->m_fn = setup.server_disconnect;

but that crashes, which I guess is kind of expected, since we're tearing down objects that are used in the server thread.

[ryanofsky]

re: #160 (comment)

In commit "test: Test disconnects during IPC calls" (84cf56a)

The previous test exercises a server disconnect. Would it be useful to either add another case, or expand it to test the server going away during a client call?

That's a good idea. It should be legitimate for an IPC call on the server to close the connection which triggered the call, or for some other thread on the server to close the connection while a slow IPC call is executing. So it would make sense to add tests covering these cases and I'm a little surprised the change you tested doesn't work.

I should look into what the cause of the problem is but I suspect it is something simple like an unchecked null pointer or uncaught exception. I am beginning to learn that there are many different ways connections can be disconnected and interrupted and in capnproto each way tends to lead to a different piece of code throwing an exception. #183 is another case found today. It seems like I should probably add broader exception handlers instead of letting errors bubble up so far.

[TheCharlatan]

For what it's worth I think the code here could be merged. Further improving exception handling does not all have to happen in here. Getting the EventLoop locking cleanups in is a clear improvement on the baseline in my eyes.

[ryanofsky]

re: #160 (comment)

For what it's worth I think the code here could be merged. Further improving exception handling does not all have to happen in here. Getting the EventLoop locking cleanups in is a clear improvement on the baseline in my eyes.

Yes that makes a lot of sense. There are more improvements to be made but I don't think it's good for this PR to keep growing forever just because the next set of improvements depend on current ones.

So I'll plan to not make any more changes here and merge in the next day or so, but I would appreciate if you or Sjors or other reviewers could look at this a little more and add acks if appropriate before then. As mentioned #172 (comment) I would really like to get away from the practice of merging PRs here without current acks.

[TheCharlatan]

ACK 84cf56a