Skip to content

refactor: fix warnings from clang-tidy-20 and bitcoin-tidy #172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 29, 2025
Merged

refactor: fix warnings from clang-tidy-20 and bitcoin-tidy #172

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c1e8c1a
clang-tidy: Fix bugprone-move-forwarding-reference errors
ryanofsky May 12, 2025
3a96cdc
clang-tidy: Fix bugprone-move-forwarding-reference error
ryanofsky May 12, 2025
57a65b8
clang-tidy: Suppress bitcoin-nontrivial-threadlocal error
ryanofsky May 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion include/mp/proxy-io.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,10 @@ void ListenConnections(EventLoop& loop, int fd, InitImpl& init)
});
}

extern thread_local ThreadContext g_thread_context;
extern thread_local ThreadContext g_thread_context; // NOLINT(bitcoin-nontrivial-threadlocal)
// Silence nonstandard bitcoin tidy error "Variable with non-trivial destructor
// cannot be thread_local" which should not be a problem on modern platforms, and
// could lead to a small memory leak at worst on older ones.
Comment on lines +638 to +640
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmh, the original PR that triggered the introduction of this check (bitcoin/bitcoin#30095), links to a freebsd bug that seems to have been fixed by now.
@vasild is this check even still relevant at all?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that we try to avoid it purely out of style/preference: bitcoin/bitcoin#30095 (comment). But no strong opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've read through the history again of it again and came to the same conclusion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #172 (comment)

My understanding is that we try to avoid it purely out of style/preference: bitcoin/bitcoin#30095 (comment). But no strong opinion.

Agree in general it's good to avoid thread_local destructors. Code will be more clear and explicit if it joins threads and deletes any state associated with them after they are joined, instead adding isolated thread_local variables that get initialized and cleaned up less deterministically.

But in this case, the libmultiprocess library needs to track state associated with threads that it doesn't have any control of.

In the bitcoin codebase, node, wallet, gui, and RPC threads are all able to make IPC calls, and the IPC implementation needs to track information about threads that make IPC calls so it can guarantee calls from the same local thread always run on the same remote thread, and guarantee that if the remote thread makes a callback it will run on the same local thread (otherwise things like recursive locks will not work). The IPC implementation also should be able to free state associated with threads when they are destroyed, so it uses a thread_local struct to do all these things.

I think keeping the bitcoin-tidy check but allowing use of a thread_local destructor here to detect when unmanaged external threads are destroyed is a reasonable fix. Other possible fixes not relying on thread_local might be possible too, but would involve other tradeoffs: they might rely on OS specific calls instead, or change the memory model to delete state when connections are closed not when threads are destroyed, or change the threading model to not always run calls from same IPC client thread on same IPC server thread. These fixes would add complexity other places, so I don't think would be preferable.

Copy link
Member

@Sjors Sjors May 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guarantee calls from the same local thread always run on the same remote thread, and guarantee that if the remote thread makes a callback it will run on the same local thread

I assume you mean that local threads correspond one to one with remote threads, perhaps even sharing the same name, but they're not actually the same thread given that they're in a different process?

Copy link
Member

@Sjors Sjors May 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious to learn how non-multiprocess clients like #174 experience this one-to-one mapping.

Even in my own Sv2 sidecar application I haven't really paid attention to this. I guess it just works(tm).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my understanding is that modern FreeBSD versions should be fine with this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #172 (comment)

I'm curious to learn how non-multiprocess clients like #174 experience this one-to-one mapping.

When you are using the libmultiprocess library as the IPC client, it forces you into the 1:1 threading model. The first time you make an IPC call from a local thread, the library will automatically make a remote ThreadMap.makeThread call to execute the current IPC request and any future requests on.

But if you are using the Cap'n Proto interface directly from rust or c++ without libmultiprocess, you don't need to have a 1:1 threading model. You can create remote threads freely and make any IPC method call which takes a Context argument run on whichever thread you specify in the Context.thread field.

re: #172 (comment)

I assume you mean that local threads correspond one to one with remote threads, perhaps even sharing the same name, but they're not actually the same thread given that they're in a different process?

Yes that's right. The sentence is also trying to say if a local thread makes an IPC call, and during that IPC call the remote thread needs to call back into the local process, the callback will be executed on literally the same thread that made the first IPC call and is currently waiting for the response.

It's important for this to happen, for example, if the thread making the first IPC call locks a recursive mutex before the call. A callback from the server to the client will need to run on literally the same client thread to avoid deadlocking if it locks the mutex recursively.


} // namespace mp

Expand Down