Skip to content

Commit 191e997

Browse files
committed
web: replace sleep_for with strand-based fence in shutdown broadcast
The shutdown path used sleep_for(100ms) to give ASIO threads time to flush the shutdown broadcast before tearing down the io_context. This is fragile: 100ms is arbitrary and provides no delivery guarantee. Add broadcastAndWait() to SessionRegistry which posts a fence lambda to each session's strand after the write lambda. Since strands are FIFO, the fence executes after the write is queued. A condition variable collects all fence completions with a 2-second timeout as a safety valve. Each session now registers a PostFn alongside its SendFn. The PostFn captures the session's strand and calls net::post(strand_, fn). If the session is already destroyed (weak_ptr expired), the PostFn calls the fence immediately so the counter still decrements. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
1 parent b9c0ec7 commit 191e997

5 files changed

Lines changed: 191 additions & 29 deletions

File tree

src/web/src/web.cpp

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -422,18 +422,29 @@ void WebSocketSession::on_accept(beast::error_code ec)
422422
// browsers reject with "A server must not mask any frames".
423423
if (viewer_hook_ != nullptr) {
424424
auto weak_self = std::weak_ptr<WebSocketSession>(shared_from_this());
425-
viewer_token_
426-
= viewer_hook_->sessions().add([weak_self](const std::string& json) {
427-
auto self = weak_self.lock();
428-
if (!self) {
429-
return;
430-
}
431-
WebSocketResponse resp;
432-
resp.id = 0;
433-
resp.type = 0; // JSON
434-
resp.payload.assign(json.begin(), json.end());
435-
self->queue_response(resp);
436-
});
425+
viewer_token_ = viewer_hook_->sessions().add(
426+
// SendFn — queue a JSON push message on this session's write queue.
427+
[weak_self](const std::string& json) {
428+
auto self = weak_self.lock();
429+
if (!self) {
430+
return;
431+
}
432+
WebSocketResponse resp;
433+
resp.id = 0;
434+
resp.type = 0; // JSON
435+
resp.payload.assign(json.begin(), json.end());
436+
self->queue_response(resp);
437+
},
438+
// PostFn — post an arbitrary callable onto this session's strand.
439+
// Used by broadcastAndWait() to fence after the write is queued.
440+
[weak_self](std::function<void()> fn) {
441+
auto self = weak_self.lock();
442+
if (!self) {
443+
fn(); // session gone — signal fence immediately
444+
return;
445+
}
446+
net::post(self->strand_, std::move(fn));
447+
});
437448
}
438449

439450
// Build search indices in the background; tiles render without shapes

src/web/src/web_serve.cpp

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -210,12 +210,13 @@ void WebServer::waitForStop()
210210
lock.unlock();
211211

212212
// Notify connected browsers so they can show "Server stopped" and
213-
// disable auto-reconnect. broadcast() posts async writes via
214-
// net::post() on each session's strand, so we sleep briefly to
215-
// let the ASIO threads flush them before stop() tears everything down.
213+
// disable auto-reconnect. broadcastAndWait() posts the message and
214+
// then waits for a strand-fence on each session, guaranteeing the
215+
// write is queued before stop() tears down the io_context.
216216
if (viewer_hook_) {
217-
viewer_hook_->sessions().broadcast(R"({"type":"shutdown"})");
218-
std::this_thread::sleep_for(std::chrono::milliseconds(100));
217+
constexpr auto kShutdownFlushTimeout = std::chrono::seconds(2);
218+
viewer_hook_->sessions().broadcastAndWait(R"({"type":"shutdown"})",
219+
kShutdownFlushTimeout);
219220
}
220221

221222
stop();

src/web/src/web_viewer_hook.cpp

Lines changed: 66 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,13 @@ constexpr auto kMaxPauseTimeout = std::chrono::minutes(10);
3030
// SessionRegistry
3131
//------------------------------------------------------------------------------
3232

33-
std::size_t SessionRegistry::add(SendFn send)
33+
std::size_t SessionRegistry::add(SendFn send, PostFn post)
3434
{
3535
std::size_t token;
3636
{
3737
std::lock_guard<std::mutex> lock(mutex_);
3838
token = next_token_++;
39-
senders_.emplace(token, std::move(send));
39+
senders_.emplace(token, SessionCallbacks{std::move(send), std::move(post)});
4040
}
4141
client_cv_.notify_all();
4242
return token;
@@ -75,21 +75,78 @@ void SessionRegistry::notifyClientWaiters()
7575

7676
void SessionRegistry::broadcast(const std::string& json)
7777
{
78-
// Copy the senders out under the lock so we don't hold it while
79-
// invoking callbacks (which may take session-level locks of their own).
80-
std::vector<SendFn> to_send;
78+
// Copy the callbacks out under the lock so we don't hold it while
79+
// invoking them (which may take session-level locks of their own).
80+
std::vector<SessionCallbacks> to_send;
8181
{
8282
std::lock_guard<std::mutex> lock(mutex_);
8383
to_send.reserve(senders_.size());
84-
for (const auto& [_, fn] : senders_) {
85-
to_send.push_back(fn);
84+
for (const auto& [_, cb] : senders_) {
85+
to_send.push_back(cb);
8686
}
8787
}
88-
for (const auto& fn : to_send) {
89-
fn(json);
88+
for (const auto& cb : to_send) {
89+
cb.send(json);
9090
}
9191
}
9292

93+
bool SessionRegistry::broadcastAndWait(const std::string& json,
94+
std::chrono::milliseconds timeout)
95+
{
96+
// Copy the callbacks out under the lock.
97+
std::vector<SessionCallbacks> to_send;
98+
{
99+
std::lock_guard<std::mutex> lock(mutex_);
100+
to_send.reserve(senders_.size());
101+
for (const auto& [_, cb] : senders_) {
102+
to_send.push_back(cb);
103+
}
104+
}
105+
106+
// Count sessions that support fencing (have a PostFn).
107+
std::size_t fence_count = 0;
108+
for (const auto& cb : to_send) {
109+
if (cb.post) {
110+
++fence_count;
111+
}
112+
}
113+
114+
if (fence_count == 0) {
115+
// No fenceable sessions — fire-and-forget like broadcast().
116+
for (const auto& cb : to_send) {
117+
cb.send(json);
118+
}
119+
return true;
120+
}
121+
122+
// Shared state for the fence: each session's PostFn decrements the
123+
// counter and notifies when all fences have fired.
124+
struct FenceState
125+
{
126+
std::mutex mutex;
127+
std::condition_variable cv;
128+
std::size_t remaining;
129+
};
130+
auto state = std::make_shared<FenceState>();
131+
state->remaining = fence_count;
132+
133+
for (const auto& cb : to_send) {
134+
cb.send(json);
135+
if (cb.post) {
136+
cb.post([state]() {
137+
std::lock_guard<std::mutex> lock(state->mutex);
138+
if (--state->remaining == 0) {
139+
state->cv.notify_one();
140+
}
141+
});
142+
}
143+
}
144+
145+
std::unique_lock<std::mutex> lock(state->mutex);
146+
return state->cv.wait_for(
147+
lock, timeout, [&state]() { return state->remaining == 0; });
148+
}
149+
93150
//------------------------------------------------------------------------------
94151
// WebViewerHook
95152
//------------------------------------------------------------------------------

src/web/src/web_viewer_hook.h

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#pragma once
55

66
#include <atomic>
7+
#include <chrono>
78
#include <condition_variable>
89
#include <cstddef>
910
#include <functional>
@@ -26,11 +27,14 @@ class SessionRegistry
2627
{
2728
public:
2829
using SendFn = std::function<void(const std::string& json)>;
30+
// Posts an arbitrary callable onto the session's strand.
31+
using PostFn = std::function<void(std::function<void()>)>;
2932
using WaitInterruptFn = std::function<bool()>;
3033

3134
// Register a send callback. Returns a token the caller must pass to
32-
// remove() during teardown.
33-
std::size_t add(SendFn send);
35+
// remove() during teardown. The optional PostFn lets broadcastAndWait()
36+
// post a fence lambda onto the session's strand.
37+
std::size_t add(SendFn send, PostFn post = {});
3438
void remove(std::size_t token);
3539

3640
// True if at least one session is registered.
@@ -47,11 +51,23 @@ class SessionRegistry
4751
// Deliver the JSON string to every currently-registered session.
4852
void broadcast(const std::string& json);
4953

54+
// Like broadcast(), but waits until every session's strand has executed
55+
// the queued write (or timeout expires). Returns true if all fences
56+
// completed, false on timeout.
57+
bool broadcastAndWait(const std::string& json,
58+
std::chrono::milliseconds timeout);
59+
5060
private:
61+
struct SessionCallbacks
62+
{
63+
SendFn send;
64+
PostFn post; // may be empty for legacy callers
65+
};
66+
5167
mutable std::mutex mutex_;
5268
mutable std::condition_variable client_cv_;
5369
std::size_t next_token_ = 1;
54-
std::unordered_map<std::size_t, SendFn> senders_;
70+
std::unordered_map<std::size_t, SessionCallbacks> senders_;
5571
};
5672

5773
// The web viewer's bridge to gui::Gui. Installed as the Gui's

src/web/test/cpp/TestDebugGraphics.cpp

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,83 @@ static std::size_t registerDummyClient(WebViewerHook& hook)
109109
return hook.sessions().add([](const std::string&) {});
110110
}
111111

112+
TEST(SessionRegistryTest, BroadcastAndWaitNoSessions)
113+
{
114+
SessionRegistry registry;
115+
// No sessions registered — should return true immediately.
116+
EXPECT_TRUE(registry.broadcastAndWait("{}", std::chrono::milliseconds(100)));
117+
}
118+
119+
TEST(SessionRegistryTest, BroadcastAndWaitFencesComplete)
120+
{
121+
SessionRegistry registry;
122+
std::string received;
123+
124+
// Register a session whose PostFn invokes the fence on a background thread
125+
// (simulating a real strand dispatch).
126+
auto token = registry.add(
127+
[&received](const std::string& json) { received = json; },
128+
[](std::function<void()> fence) {
129+
std::thread([fence = std::move(fence)]() { fence(); }).detach();
130+
});
131+
132+
EXPECT_TRUE(registry.broadcastAndWait(R"({"type":"shutdown"})",
133+
std::chrono::seconds(2)));
134+
EXPECT_EQ(received, R"({"type":"shutdown"})");
135+
registry.remove(token);
136+
}
137+
138+
TEST(SessionRegistryTest, BroadcastAndWaitTimesOut)
139+
{
140+
SessionRegistry registry;
141+
142+
// Register a session whose PostFn silently drops the fence.
143+
auto token = registry.add(
144+
[](const std::string&) {},
145+
[](std::function<void()> /*fence*/) { /* never called */ });
146+
147+
const auto t0 = std::chrono::steady_clock::now();
148+
EXPECT_FALSE(registry.broadcastAndWait("{}", std::chrono::milliseconds(200)));
149+
const auto elapsed = std::chrono::steady_clock::now() - t0;
150+
151+
// Should have waited approximately the timeout, not longer.
152+
EXPECT_GE(elapsed, std::chrono::milliseconds(150));
153+
EXPECT_LT(elapsed, std::chrono::seconds(2));
154+
registry.remove(token);
155+
}
156+
157+
TEST(SessionRegistryTest, BroadcastAndWaitDeadSession)
158+
{
159+
SessionRegistry registry;
160+
std::atomic<bool> send_called{false};
161+
162+
// Register a session whose PostFn calls the fence immediately
163+
// (simulating a dead session that signals right away).
164+
auto token
165+
= registry.add([&send_called](const std::string&) { send_called = true; },
166+
[](std::function<void()> fence) { fence(); });
167+
168+
EXPECT_TRUE(registry.broadcastAndWait("{}", std::chrono::milliseconds(100)));
169+
EXPECT_TRUE(send_called);
170+
registry.remove(token);
171+
}
172+
173+
TEST(SessionRegistryTest, BroadcastAndWaitLegacySendOnly)
174+
{
175+
SessionRegistry registry;
176+
std::string received;
177+
178+
// Register with SendFn only (no PostFn) — legacy path.
179+
auto token
180+
= registry.add([&received](const std::string& json) { received = json; });
181+
182+
// Should return true immediately since there are no fenceable sessions.
183+
EXPECT_TRUE(registry.broadcastAndWait(R"({"ok":true})",
184+
std::chrono::milliseconds(100)));
185+
EXPECT_EQ(received, R"({"ok":true})");
186+
registry.remove(token);
187+
}
188+
112189
TEST(SessionRegistryTest, WaitForClientCanBeInterrupted)
113190
{
114191
SessionRegistry registry;

0 commit comments

Comments
 (0)