Skip to content

Navigation Menu

Sign in
Appearance settings
Sign in
Sign up
Appearance settings

Commit f5c15ce

Browse filesBrowse the repository at this point in the historyBrowse files
ryanofskyryanofsky
committed
Merge bitcoin-core/libmultiprocess#323: refactor: access ThreadContext through CurrentThread(), ci: switch Bitcoin Core to master
66298c7 ci: Switch back to Bitcoin Core's master branch (Hennadii Stepanov) 86b4810 refactor: access ThreadContext through CurrentThread() (Sjors Provoost) Pull request description: This is a minimal subset from bitcoin#318 needed to match the shim introduced by bitcoin#35084. Should unblock bitcoin#322, see bitcoin-core/libmultiprocess#322 (comment). Include bitcoin#322 here, because CI will not pass now since bitcoin#35084 merged. ACKs for top commit: xyzconstant: ACK 66298c7 hebasto: ACK 66298c7. ViniciusCestarii: ACK 66298c7 ryanofsky: Code review ACK 66298c7. This nicely separates the `CurrentThread` refactoring from the mingw bug workaround. But it's misleading to call this a minimal fix for compatibility, because a minimal fix would just be: Tree-SHA512: 04cdb9f72c01279a7d9d8bd5735be5114dc6c37a5c66c5cab3e199d48c42da30d2db4c0bcd4f3bcf5508a04a52c78d63945b7ea9b605624353478872d38c5ebe
2 parents 3f221b5 + 66298c7 commit f5c15ce
Copy full SHA for f5c15ce

6 files changed

+51-27Lines changed: 51 additions & 27 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎.github/workflows/bitcoin-core-ci.yml‎

Copy file name to clipboardExpand all lines: .github/workflows/bitcoin-core-ci.yml
+1-2Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,7 @@ concurrency:
1818

1919
env:
2020
BITCOIN_REPO: bitcoin/bitcoin
21-
# Temporary: use PR #35084 until it merges; revert to refs/heads/master after
22-
BITCOIN_CORE_REF: refs/pull/35084/merge
21+
BITCOIN_CORE_REF: refs/heads/master
2322
LLVM_VERSION: 22
2423
LIBCXX_DIR: /tmp/libcxx-build/
2524

Collapse file

‎include/mp/proxy-io.h‎

Copy file name to clipboardExpand all lines: include/mp/proxy-io.h
+21-1Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -694,7 +694,7 @@ using ConnThread = ConnThreads::iterator;
694694
// inserted bool.
695695
std::tuple<ConnThread, bool> SetThread(GuardedRef<ConnThreads> threads, Connection* connection, const std::function<Thread::Client()>& make_thread);
696696

697-
//! The thread_local ThreadContext g_thread_context struct provides information
697+
//! The thread_local ThreadContext struct (see CurrentThread()) provides information
698698
//! about individual threads and a way of communicating between them. Because
699699
//! it's a thread local struct, each ThreadContext instance is initialized by
700700
//! the thread that owns it.
@@ -934,6 +934,26 @@ extern thread_local ThreadContext g_thread_context; // NOLINT(bitcoin-nontrivial
934934
// cannot be thread_local" which should not be a problem on modern platforms, and
935935
// could lead to a small memory leak at worst on older ones.
936936

937+
//! Return the current thread's ThreadContext.
938+
//!
939+
//! Why per-thread state is needed at all: libmultiprocess has no control over
940+
//! which threads the C++ application uses to call ProxyClient methods after
941+
//! the proxy objects are returned to it. The thread-mapping model (see
942+
//! "Thread Mapping" in doc/design.md) gives each application thread making
943+
//! IPC calls a dedicated server-side thread that executes its requests, so
944+
//! thread-local state and recursive mutexes work as expected across the
945+
//! process boundary and callbacks from the server run on the originating
946+
//! client thread. The client-side handles for those dedicated server threads
947+
//! (the ProxyClient<Thread> objects returned by ThreadMap.makeThread, stored
948+
//! per connection in the request_threads / callback_threads maps below) are
949+
//! state that must be keyed implicitly by the calling thread, and must be
950+
//! released when the client thread exits so the corresponding server threads
951+
//! are freed. A thread_local object is the C++ mechanism that provides both
952+
//! of these: per-thread storage plus a destructor that runs at thread exit
953+
//! (the C equivalent would be a pthread key destructor). This is why
954+
//! ThreadContext is thread_local and why its destructor is nontrivial.
955+
ThreadContext& CurrentThread();
956+
937957
} // namespace mp
938958

939959
#endif // MP_PROXY_IO_H
Collapse file

‎include/mp/proxy-types.h‎

Copy file name to clipboardExpand all lines: include/mp/proxy-types.h
+7-7Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -691,9 +691,9 @@ void serverDestroy(Server& server)
691691
template <typename ProxyClient, typename GetRequest, typename... FieldObjs>
692692
void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, FieldObjs&&... fields)
693693
{
694-
if (!g_thread_context.waiter) {
695-
assert(g_thread_context.thread_name.empty());
696-
g_thread_context.thread_name = ThreadName(proxy_client.m_context.loop->m_exe_name);
694+
if (!CurrentThread().waiter) {
695+
assert(CurrentThread().thread_name.empty());
696+
CurrentThread().thread_name = ThreadName(proxy_client.m_context.loop->m_exe_name);
697697
// If next assert triggers, it means clientInvoke is being called from
698698
// the capnp event loop thread. This can happen when a ProxyServer
699699
// method implementation that runs synchronously on the event loop
@@ -702,13 +702,13 @@ void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, Fiel
702702
// run asynchronously off the event loop thread. This is easy to fix by
703703
// just adding a 'context :Proxy.Context' argument to the capnp method
704704
// declaration so the server method runs in a dedicated thread.
705-
assert(!g_thread_context.loop_thread);
706-
g_thread_context.waiter = std::make_unique<Waiter>();
705+
assert(!CurrentThread().loop_thread);
706+
CurrentThread().waiter = std::make_unique<Waiter>();
707707
MP_LOGPLAIN(*proxy_client.m_context.loop, Log::Info)
708-
<< "{" << g_thread_context.thread_name
708+
<< "{" << CurrentThread().thread_name
709709
<< "} IPC client first request from current thread, constructing waiter";
710710
}
711-
ThreadContext& thread_context{g_thread_context};
711+
ThreadContext& thread_context{CurrentThread()};
712712
std::optional<ClientInvokeContext> invoke_context; // Must outlive waiter->wait() call below
713713
std::exception_ptr exception;
714714
std::string kj_exception;
Collapse file

‎include/mp/type-context.h‎

Copy file name to clipboardExpand all lines: include/mp/type-context.h
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
9393
// call. In this case, the callbackThread value should point
9494
// to the same thread already in the map, so there is no
9595
// need to update the map.
96-
auto& thread_context = g_thread_context;
96+
auto& thread_context = CurrentThread();
9797
auto& request_threads = thread_context.request_threads;
9898
ConnThread request_thread;
9999
bool inserted{false};
Collapse file

‎src/mp/proxy.cpp‎

Copy file name to clipboardExpand all lines: src/mp/proxy.cpp
+19-14Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,11 @@ namespace mp {
4242

4343
thread_local ThreadContext g_thread_context; // NOLINT(bitcoin-nontrivial-threadlocal)
4444

45+
ThreadContext& CurrentThread()
46+
{
47+
return g_thread_context;
48+
}
49+
4550
Stream MakeStream(EventLoop&loop, SocketId socket)
4651
{
4752
Stream stream;
@@ -283,9 +288,9 @@ EventLoop::~EventLoop()
283288

284289
void EventLoop::loop()
285290
{
286-
assert(!g_thread_context.loop_thread);
287-
g_thread_context.loop_thread = true;
288-
KJ_DEFER(g_thread_context.loop_thread = false);
291+
assert(!CurrentThread().loop_thread);
292+
CurrentThread().loop_thread = true;
293+
KJ_DEFER(CurrentThread().loop_thread = false);
289294

290295
{
291296
const Lock lock(m_mutex);
@@ -479,11 +484,11 @@ kj::Promise<void> ProxyServer<ThreadMap>::makePool(MakePoolContext context)
479484
const std::string thread_name = "pool/" + std::to_string(i);
480485
std::promise<ThreadContext*> thread_context;
481486
std::thread thread([&loop, &thread_context, thread_name]() {
482-
g_thread_context.thread_name = ThreadName(loop.m_exe_name) + " (" + thread_name + ")";
483-
g_thread_context.waiter = std::make_unique<Waiter>();
484-
Lock lock(g_thread_context.waiter->m_mutex);
485-
thread_context.set_value(&g_thread_context);
486-
g_thread_context.waiter->wait(lock, [] { return !g_thread_context.waiter; });
487+
CurrentThread().thread_name = ThreadName(loop.m_exe_name) + " (" + thread_name + ")";
488+
CurrentThread().waiter = std::make_unique<Waiter>();
489+
Lock lock(CurrentThread().waiter->m_mutex);
490+
thread_context.set_value(&CurrentThread());
491+
CurrentThread().waiter->wait(lock, [] { return !CurrentThread().waiter; });
487492
});
488493
auto thread_server = kj::heap<ProxyServer<Thread>>(m_connection, *thread_context.get_future().get(), std::move(thread));
489494
m_connection.m_thread_pool.push_back({m_connection.m_threads.add(kj::mv(thread_server))});
@@ -498,14 +503,14 @@ kj::Promise<void> ProxyServer<ThreadMap>::makeThread(MakeThreadContext context)
498503
const std::string from = context.getParams().getName();
499504
std::promise<ThreadContext*> thread_context;
500505
std::thread thread([&loop, &thread_context, from]() {
501-
g_thread_context.thread_name = ThreadName(loop.m_exe_name) + " (from " + from + ")";
502-
g_thread_context.waiter = std::make_unique<Waiter>();
503-
Lock lock(g_thread_context.waiter->m_mutex);
504-
thread_context.set_value(&g_thread_context);
506+
CurrentThread().thread_name = ThreadName(loop.m_exe_name) + " (from " + from + ")";
507+
CurrentThread().waiter = std::make_unique<Waiter>();
508+
Lock lock(CurrentThread().waiter->m_mutex);
509+
thread_context.set_value(&CurrentThread());
505510
if (loop.testing_hook_makethread_created) loop.testing_hook_makethread_created();
506511
// Wait for shutdown signal from ProxyServer<Thread> destructor (signal
507512
// is just waiter getting set to null.)
508-
g_thread_context.waiter->wait(lock, [] { return !g_thread_context.waiter; });
513+
CurrentThread().waiter->wait(lock, [] { return !CurrentThread().waiter; });
509514
});
510515
auto thread_server = kj::heap<ProxyServer<Thread>>(m_connection, *thread_context.get_future().get(), std::move(thread));
511516
auto thread_client = m_connection.m_threads.add(kj::mv(thread_server));
@@ -517,7 +522,7 @@ std::atomic<int> server_reqs{0};
517522

518523
std::string LongThreadName(const char* exe_name)
519524
{
520-
return g_thread_context.thread_name.empty() ? ThreadName(exe_name) : g_thread_context.thread_name;
525+
return CurrentThread().thread_name.empty() ? ThreadName(exe_name) : CurrentThread().thread_name;
521526
}
522527

523528
kj::StringPtr KJ_STRINGIFY(Log v)
Collapse file

‎test/mp/test/test.cpp‎

Copy file name to clipboardExpand all lines: test/mp/test/test.cpp
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -462,7 +462,7 @@ KJ_TEST("Make simultaneous IPC calls on single remote thread")
462462
// that will be used for the test.
463463
setup.server->m_impl->m_fn = [&] {};
464464
foo->callFnAsync();
465-
ThreadContext& tc{g_thread_context};
465+
ThreadContext& tc{CurrentThread()};
466466
Thread::Client *callback_thread, *request_thread;
467467
foo->m_context.loop->sync([&] {
468468
Lock lock(tc.waiter->m_mutex);
@@ -516,7 +516,7 @@ KJ_TEST("Call async IPC method dispatched to pool thread")
516516
foo->initThreadMap();
517517
setup.server->m_impl->m_int_fn = [](int n) { return n * 2; };
518518

519-
ThreadContext& tc{g_thread_context};
519+
ThreadContext& tc{CurrentThread()};
520520
std::atomic<size_t> running{3};
521521
std::promise<void> pool_ready;
522522
foo->m_context.loop->sync([&] {

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.