Skip to content

Commit 8beb60b

Browse files
authored
fix(ipc): auto-claim shm client slots (#24236)
## Summary - make MPSC SHM clients self-allocate a free producer slot when no explicit client id is provided - pass omitted TS/NAPI client ids through to the native auto-claim path instead of defaulting to slot 0 - add C++ and world-state regression tests for multi-client SHM response correlation and clean slot reuse ## Why Multiple SHM clients were defaulting to producer slot 0. In the wsdb/AVM topology that means separate clients can share one request/response ring pair and steal or mis-correlate responses. The fix adds a small owner table to the existing MPSC doorbell mapping so clients can atomically claim distinct slots. This intentionally does not try to recover stale in-flight requests left by a crashed client. Clean disconnect releases the claimed slot for reuse. ## Validation - cmake --build build --target ipc_runtime_tests -j - ./build/ipc_runtime_tests --gtest_filter='ShmTest.MpscReactorMultiClientResponseRouting:ShmTest.MpscReactorAutoClaimMultiClient:ShmTest.MpscSlotFreedOnDisconnectAndReclaimed' Note: the fresh next-based worktree had no yarn install state, so the TS world-state test is left for CI here. It passed earlier in the existing built ipc-5 worktree before this was split into a standalone PR.
2 parents d69ab88 + 4a3488a commit 8beb60b

10 files changed

Lines changed: 530 additions & 13 deletions

File tree

ipc-codegen/src/typescript_package_codegen.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -311,7 +311,9 @@ async function connectClient(
311311
}
312312
${supportsShm ? ` if (transport === 'shm') {
313313
return createNapiShmAsyncClient(ipcPath.replace(/\\.shm$/, ''), {
314-
clientId: options.clientId ?? 0,
314+
// Pass clientId through as-is: when unset, the client self-allocates a
315+
// free producer slot (don't default to 0, which aliases every client).
316+
clientId: options.clientId,
315317
customAddonPath: options.napiPath,
316318
});
317319
}

ipc-runtime/cpp/ipc_runtime/ipc_client.hpp

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,19 @@
22

33
#include <cstddef>
44
#include <cstdint>
5+
#include <limits>
56
#include <memory>
67
#include <span>
78
#include <string>
89
#include <sys/types.h>
910

1011
namespace ipc {
1112

13+
// Passed as the MPSC-SHM client id to mean "atomically claim a free slot on
14+
// connect" rather than pinning a specific one. This is the default for
15+
// make_client / create_mpsc_shm; an explicit id is only used by tests.
16+
inline constexpr std::size_t kAutoClientId = std::numeric_limits<std::size_t>::max();
17+
1218
/**
1319
* @brief Abstract interface for IPC client
1420
*
@@ -84,7 +90,7 @@ class IpcClient {
8490
static std::unique_ptr<IpcClient> create_shm(const std::string& base_name);
8591
// Multi-producer SHM: one request ring per client slot and one response
8692
// ring per client slot. This is what make_client("*.shm") selects.
87-
static std::unique_ptr<IpcClient> create_mpsc_shm(const std::string& base_name, size_t client_id);
93+
static std::unique_ptr<IpcClient> create_mpsc_shm(const std::string& base_name, size_t client_id = kAutoClientId);
8894
};
8995

9096
/**
@@ -95,12 +101,12 @@ class IpcClient {
95101
* - "*.shm" → IpcClient::create_mpsc_shm(<basename>, client_id)
96102
*
97103
* Returns nullptr if the suffix is not recognised. `shm_client_id` is only
98-
* consulted for the SHM path; for MPSC-SHM, each connecting client picks a
99-
* distinct slot (0..max_clients-1).
104+
* consulted for the SHM path; it defaults to kAutoClientId, so each connecting
105+
* client atomically claims a distinct free slot (0..max_clients-1) on connect.
100106
*
101107
* @param input_path Path passed by the caller (often a CLI flag).
102-
* @param shm_client_id Client slot to claim in MPSC-SHM mode. Ignored for UDS.
108+
* @param shm_client_id MPSC-SHM slot to pin, or kAutoClientId to self-allocate. Ignored for UDS.
103109
*/
104-
std::unique_ptr<IpcClient> make_client(const std::string& input_path, std::size_t shm_client_id = 0);
110+
std::unique_ptr<IpcClient> make_client(const std::string& input_path, std::size_t shm_client_id = kAutoClientId);
105111

106112
} // namespace ipc

ipc-runtime/cpp/ipc_runtime/mpsc_shm_client.hpp

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,13 @@ namespace ipc {
2424
*/
2525
class MpscShmClient : public IpcClient {
2626
public:
27+
// client_id == kAutoClientId (the default for make_client / create_mpsc_shm)
28+
// makes connect() atomically claim a free producer slot from the server's
29+
// shared slot table, so callers don't have to coordinate unique ids. An
30+
// explicit id pins a specific slot (used by tests).
2731
MpscShmClient(std::string base_name, size_t client_id)
2832
: base_name_(std::move(base_name))
33+
, auto_claim_(client_id == kAutoClientId)
2934
, client_id_(client_id)
3035
{}
3136

@@ -48,6 +53,14 @@ class MpscShmClient : public IpcClient {
4853

4954
for (size_t attempt = 0; attempt < max_attempts; ++attempt) {
5055
try {
56+
// Self-allocate a producer slot when no explicit id was given.
57+
// The claim is held for the connection's lifetime and released by
58+
// close()/destruction so the slot can be reused.
59+
if (auto_claim_) {
60+
slot_claim_ = MpscSlotClaim::claim(base_name_ + "_req");
61+
client_id_ = slot_claim_->id();
62+
}
63+
5164
// Connect as producer to the MPSC request system
5265
producer_ = MpscProducer::connect(base_name_ + "_req", client_id_);
5366

@@ -59,6 +72,7 @@ class MpscShmClient : public IpcClient {
5972
} catch (...) {
6073
producer_.reset();
6174
response_ring_.reset();
75+
slot_claim_.reset();
6276
if (attempt + 1 == max_attempts) {
6377
return false;
6478
}
@@ -112,6 +126,7 @@ class MpscShmClient : public IpcClient {
112126
{
113127
producer_.reset();
114128
response_ring_.reset();
129+
slot_claim_.reset(); // release our producer slot for reuse
115130
}
116131

117132
void wakeup() override
@@ -123,7 +138,9 @@ class MpscShmClient : public IpcClient {
123138

124139
private:
125140
std::string base_name_;
141+
bool auto_claim_;
126142
size_t client_id_;
143+
std::optional<MpscSlotClaim> slot_claim_;
127144
std::optional<MpscProducer> producer_;
128145
std::optional<SpscShm> response_ring_;
129146
};

ipc-runtime/cpp/ipc_runtime/shm.test.cpp

Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -505,4 +505,218 @@ TEST(ShmTest, MpscReactorPipelinedConcurrencyAndOrder)
505505
server->close();
506506
}
507507

508+
TEST(ShmTest, MpscReactorMultiClientResponseRouting)
509+
{
510+
constexpr uint32_t NUM_CLIENTS = 4;
511+
constexpr uint32_t K = 128;
512+
constexpr size_t RING_SIZE = 64UL * 1024;
513+
514+
std::string base_name = "shm_mpsc_multi_" + std::to_string(getpid());
515+
auto server = IpcServer::create_mpsc_shm(base_name, NUM_CLIENTS, RING_SIZE, RING_SIZE);
516+
ASSERT_TRUE(server->listen()) << "MPSC multi-client server failed to listen";
517+
518+
ReactorTestPool pool(8);
519+
std::thread server_thread([&]() {
520+
server->run_reactor([&pool](int, std::span<const uint8_t> req, IpcServer::Respond respond) {
521+
std::vector<uint8_t> r(req.begin(), req.end());
522+
pool.enqueue([r = std::move(r), respond = std::move(respond)]() mutable {
523+
uint32_t seq = 0;
524+
std::memcpy(&seq, r.data() + sizeof(uint32_t), sizeof(uint32_t));
525+
std::this_thread::sleep_for(std::chrono::microseconds(20 * (seq % 8)));
526+
respond(std::move(r));
527+
});
528+
});
529+
});
530+
std::this_thread::sleep_for(std::chrono::milliseconds(100));
531+
532+
std::atomic<int> wrong_client{ 0 };
533+
std::atomic<int> out_of_order{ 0 };
534+
std::atomic<int> stalls{ 0 };
535+
536+
auto run_client = [&](uint32_t c) {
537+
auto client = IpcClient::create_mpsc_shm(base_name, c);
538+
if (!client->connect()) {
539+
stalls++;
540+
return;
541+
}
542+
for (uint32_t s = 0; s < K; s++) {
543+
uint32_t msg[2] = { c, s };
544+
while (!client->send(msg, sizeof(msg), 100'000'000ULL)) {
545+
}
546+
}
547+
for (uint32_t s = 0; s < K; s++) {
548+
std::span<const uint8_t> resp;
549+
size_t empties = 0;
550+
while ((resp = client->receive(100'000'000ULL)).empty()) {
551+
if (++empties > 50) {
552+
stalls++;
553+
break;
554+
}
555+
}
556+
if (resp.empty()) {
557+
break;
558+
}
559+
uint32_t got[2] = { 0, 0 };
560+
std::memcpy(got, resp.data(), sizeof(got));
561+
if (got[0] != c) {
562+
wrong_client++;
563+
}
564+
if (got[1] != s) {
565+
out_of_order++;
566+
}
567+
client->release(resp.size());
568+
}
569+
client->close();
570+
};
571+
572+
std::vector<std::thread> clients;
573+
for (uint32_t c = 0; c < NUM_CLIENTS; c++) {
574+
clients.emplace_back(run_client, c);
575+
}
576+
for (auto& t : clients) {
577+
t.join();
578+
}
579+
580+
server->request_shutdown();
581+
server_thread.join();
582+
server->close();
583+
584+
EXPECT_EQ(wrong_client.load(), 0) << "responses were routed to the wrong client";
585+
EXPECT_EQ(out_of_order.load(), 0) << "responses arrived out of order within a client";
586+
EXPECT_EQ(stalls.load(), 0) << "a client stalled waiting for its responses";
587+
}
588+
589+
TEST(ShmTest, MpscReactorAutoClaimMultiClient)
590+
{
591+
constexpr uint32_t NUM_CLIENTS = 4;
592+
constexpr uint32_t K = 128;
593+
constexpr size_t RING_SIZE = 64UL * 1024;
594+
595+
std::string base_name = "shm_mpsc_autoclaim_" + std::to_string(getpid());
596+
auto server = IpcServer::create_mpsc_shm(base_name, NUM_CLIENTS, RING_SIZE, RING_SIZE);
597+
ASSERT_TRUE(server->listen()) << "MPSC auto-claim server failed to listen";
598+
599+
ReactorTestPool pool(8);
600+
std::thread server_thread([&]() {
601+
server->run_reactor([&pool](int, std::span<const uint8_t> req, IpcServer::Respond respond) {
602+
std::vector<uint8_t> r(req.begin(), req.end());
603+
pool.enqueue([r = std::move(r), respond = std::move(respond)]() mutable {
604+
uint32_t seq = 0;
605+
std::memcpy(&seq, r.data() + sizeof(uint32_t), sizeof(uint32_t));
606+
std::this_thread::sleep_for(std::chrono::microseconds(20 * (seq % 8)));
607+
respond(std::move(r));
608+
});
609+
});
610+
});
611+
std::this_thread::sleep_for(std::chrono::milliseconds(100));
612+
613+
std::atomic<int> wrong_client{ 0 };
614+
std::atomic<int> out_of_order{ 0 };
615+
std::atomic<int> stalls{ 0 };
616+
617+
auto run_client = [&](uint32_t c) {
618+
auto client = IpcClient::create_mpsc_shm(base_name);
619+
if (!client->connect()) {
620+
stalls++;
621+
return;
622+
}
623+
for (uint32_t s = 0; s < K; s++) {
624+
uint32_t msg[2] = { c, s };
625+
while (!client->send(msg, sizeof(msg), 100'000'000ULL)) {
626+
}
627+
}
628+
for (uint32_t s = 0; s < K; s++) {
629+
std::span<const uint8_t> resp;
630+
size_t empties = 0;
631+
while ((resp = client->receive(100'000'000ULL)).empty()) {
632+
if (++empties > 50) {
633+
stalls++;
634+
break;
635+
}
636+
}
637+
if (resp.empty()) {
638+
break;
639+
}
640+
uint32_t got[2] = { 0, 0 };
641+
std::memcpy(got, resp.data(), sizeof(got));
642+
if (got[0] != c) {
643+
wrong_client++;
644+
}
645+
if (got[1] != s) {
646+
out_of_order++;
647+
}
648+
client->release(resp.size());
649+
}
650+
client->close();
651+
};
652+
653+
std::vector<std::thread> clients;
654+
for (uint32_t c = 0; c < NUM_CLIENTS; c++) {
655+
clients.emplace_back(run_client, c);
656+
}
657+
for (auto& t : clients) {
658+
t.join();
659+
}
660+
661+
server->request_shutdown();
662+
server_thread.join();
663+
server->close();
664+
665+
EXPECT_EQ(wrong_client.load(), 0) << "self-allocated clients aliased onto a shared slot";
666+
EXPECT_EQ(out_of_order.load(), 0) << "responses arrived out of order within a client";
667+
EXPECT_EQ(stalls.load(), 0) << "a client stalled";
668+
}
669+
670+
TEST(ShmTest, MpscSlotFreedOnDisconnectAndReclaimed)
671+
{
672+
constexpr size_t NUM_CLIENTS = 1;
673+
constexpr size_t RING_SIZE = 16UL * 1024;
674+
675+
std::string base_name = "shm_mpsc_reuse_" + std::to_string(getpid());
676+
auto server = IpcServer::create_mpsc_shm(base_name, NUM_CLIENTS, RING_SIZE, RING_SIZE);
677+
ASSERT_TRUE(server->listen()) << "server failed to listen";
678+
679+
ReactorTestPool pool(2);
680+
std::thread server_thread([&]() {
681+
server->run_reactor([&pool](int, std::span<const uint8_t> req, IpcServer::Respond respond) {
682+
std::vector<uint8_t> r(req.begin(), req.end());
683+
pool.enqueue([r = std::move(r), respond = std::move(respond)]() mutable { respond(std::move(r)); });
684+
});
685+
});
686+
std::this_thread::sleep_for(std::chrono::milliseconds(100));
687+
688+
auto round_trip = [](IpcClient& client, uint32_t value) -> bool {
689+
if (!client.send(&value, sizeof(value), 100'000'000ULL)) {
690+
return false;
691+
}
692+
std::span<const uint8_t> resp;
693+
size_t empties = 0;
694+
while ((resp = client.receive(100'000'000ULL)).empty()) {
695+
if (++empties > 50) {
696+
return false;
697+
}
698+
}
699+
uint32_t got = 0;
700+
std::memcpy(&got, resp.data(), sizeof(got));
701+
client.release(resp.size());
702+
return got == value;
703+
};
704+
705+
{
706+
auto a = IpcClient::create_mpsc_shm(base_name);
707+
ASSERT_TRUE(a->connect()) << "client A failed to connect";
708+
EXPECT_TRUE(round_trip(*a, 0x1111)) << "client A round-trip failed";
709+
a->close();
710+
}
711+
712+
auto b = IpcClient::create_mpsc_shm(base_name);
713+
ASSERT_TRUE(b->connect()) << "client B failed to reclaim the freed slot";
714+
EXPECT_TRUE(round_trip(*b, 0x2222)) << "client B round-trip over the reused ring failed";
715+
b->close();
716+
717+
server->request_shutdown();
718+
server_thread.join();
719+
server->close();
720+
}
721+
508722
} // namespace

0 commit comments

Comments
 (0)