Skip to content

Commit 0738739

Browse files
zahariashclaude
authored andcommitted
fix(pybind): wait for in-flight SIGINT handlers before releasing the context
A process-directed SIGINT can run the handler on any thread, for example an engine worker. If that handler had loaded the target ClientContext but not yet called interrupt() when the main thread's query returned, the guard was removed and the caller could drop the last reference to the connection (when another thread had already closed it), so the store landed on freed memory. The handler now counts itself in before loading the target and out after interrupt(). The guard's destructor clears the target and then waits until the count is zero, so a handler either is waited for or finds no target. Each side stores one atomic and then loads the other, so the four operations are explicitly sequentially consistent. The counter is lock-free and the handler stays async-signal-safe. The handler also sets the received flag and copies the previous handler before counting itself out, so the wait covers those accesses as well and the next guard does not reset them under a handler it waited for. Python's chained handler is called outside the count, so the wait never depends on code outside this file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 5b5f991 commit 0738739

1 file changed

Lines changed: 24 additions & 6 deletions

File tree

‎src_cpp/py_connection.cpp‎

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
#ifndef _WIN32
99
#include <csignal>
10+
#include <thread>
1011
#endif
1112

1213
#if PY_VERSION_HEX < 0x030D0000
@@ -232,7 +233,13 @@ class SigintInterruptsQuery {
232233
#ifndef _WIN32
233234
if (installed) {
234235
sigaction(SIGINT, &previous, nullptr);
235-
target = nullptr;
236+
target.store(nullptr, std::memory_order_seq_cst);
237+
// The caller may release the context once this returns, so wait for every handler on
238+
// another thread that has counted itself in; one that counts in later sees nullptr. A
239+
// handler on this thread runs to completion before the loop resumes.
240+
while (handlersInFlight.load(std::memory_order_seq_cst) != 0) {
241+
std::this_thread::yield();
242+
}
236243
}
237244
#endif
238245
}
@@ -268,23 +275,34 @@ class SigintInterruptsQuery {
268275
// Runs in signal context, so it may only use async-signal-safe operations: lock-free atomics
269276
// and Python's own handler, which is written for signal context. ClientContext::interrupt()
270277
// is a store to an std::atomic<bool> in the engine and must stay one.
278+
//
279+
// The handler counts itself in before loading target, and the destructor clears target before
280+
// reading the count: either the destructor waits for the handler or the handler sees nullptr.
281+
// Both sides store one atomic and then load the other, which needs seq_cst; acquire/release
282+
// would let both miss. received and previous are also used before counting out, so the wait
283+
// covers them; Python's handler is called outside the count, so the wait never depends on it.
271284
static void handle(int signum, siginfo_t* info, void* ucontext) {
272-
if (auto* context = target.load()) {
285+
handlersInFlight.fetch_add(1, std::memory_order_seq_cst);
286+
if (auto* context = target.load(std::memory_order_seq_cst)) {
273287
context->interrupt();
274288
}
275289
received = true;
290+
const auto chained = previous;
291+
handlersInFlight.fetch_sub(1, std::memory_order_seq_cst);
276292
// Python's handler only records the signal; KeyboardInterrupt is raised later.
277-
if (previous.sa_flags & SA_SIGINFO) {
278-
previous.sa_sigaction(signum, info, ucontext);
279-
} else if (previous.sa_handler != SIG_DFL && previous.sa_handler != SIG_IGN) {
280-
previous.sa_handler(signum);
293+
if (chained.sa_flags & SA_SIGINFO) {
294+
chained.sa_sigaction(signum, info, ucontext);
295+
} else if (chained.sa_handler != SIG_DFL && chained.sa_handler != SIG_IGN) {
296+
chained.sa_handler(signum);
281297
}
282298
}
283299

284300
static inline std::atomic<main::ClientContext*> target{nullptr};
285301
static inline std::atomic<bool> received{false};
302+
static inline std::atomic<int> handlersInFlight{0};
286303
static_assert(std::atomic<main::ClientContext*>::is_always_lock_free);
287304
static_assert(std::atomic<bool>::is_always_lock_free);
305+
static_assert(std::atomic<int>::is_always_lock_free);
288306
static inline struct sigaction previous {};
289307
bool installed = false;
290308
#endif

0 commit comments

Comments
 (0)