Skip to content

Commit 261964b

Browse files
afrindclaude
andcommitted
relay: hop cross-exec handle teardown to the owning exec
The reciprocal peer SubscribeNamespaceHandle was acquired via a direct session->subscribeNamespace() and stored raw, so peerSubNsHandles_.erase() destroyed it on relayExec_ — running its teardown (isClosed() read + control RST) on the wrong thread while the peer session closed on its own io exec (TSAN-confirmed data race on MoQSession). Acquire it through maybeWrapPublisher so the handle is a CrossExecSubscribeNamespaceHandle bound to the session's exec, and give every cross-exec wrapper in PublisherCrossExecFilter a destructor that hops the inner handle's destruction to that exec — so destructor-driven teardown is exec-correct, not just the explicit unsubscribe/cancel calls. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 975d24a commit 261964b

3 files changed

Lines changed: 47 additions & 5 deletions

File tree

src/MoqxRelay.cpp

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1364,11 +1364,10 @@ folly::coro::Task<Publisher::SubscribeNamespaceResult> MoqxRelay::subscribeNames
13641364
// Tag with the peer's relay ID so we suppress echoing these namespaces
13651365
// back to that peer on reconnect.
13661366
auto handle = makeNamespaceBridgeHandle(weak_from_this(), session, incomingPeerID, relayExec_);
1367-
// subscribeNamespace must run on the peer session's executor.
1368-
auto recipResult = co_await folly::coro::co_withExecutor(
1369-
folly::getKeepAliveToken(session->getExecutor()),
1370-
session->subscribeNamespace(makePeerSubNs(), handle)
1371-
); // no token: reciprocal, prevents loop
1367+
// maybeWrapPublisher runs the call on the peer session's executor and wraps
1368+
// the returned handle so its teardown hops there too (no token: reciprocal).
1369+
auto recipResult =
1370+
co_await maybeWrapPublisher(relayExec_, session)->subscribeNamespace(makePeerSubNs(), handle);
13721371
if (recipResult.hasError()) {
13731372
XLOG(ERR) << "Reciprocal peer subNs failed: " << recipResult.error().reasonPhrase;
13741373
} else {

src/relay/PublisherCrossExecFilter.cpp

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,13 @@ class CrossExecSubscriptionHandle : public moxygen::SubscriptionHandle {
2222
)
2323
: inner_(std::move(inner)), exec_(exec) {}
2424

25+
~CrossExecSubscriptionHandle() override {
26+
// Inner dtor may touch session state; destroy it on exec_, not the dropping thread.
27+
if (inner_) {
28+
exec_->add([inner = std::move(inner_)]() mutable {});
29+
}
30+
}
31+
2532
const moxygen::SubscribeOk& subscribeOk() const override { return inner_->subscribeOk(); }
2633

2734
void unsubscribe() override {
@@ -48,6 +55,13 @@ class CrossExecFetchHandle : public moxygen::Publisher::FetchHandle {
4855
)
4956
: inner_(std::move(inner)), exec_(exec) {}
5057

58+
~CrossExecFetchHandle() override {
59+
// Inner dtor may touch session state; destroy it on exec_, not the dropping thread.
60+
if (inner_) {
61+
exec_->add([inner = std::move(inner_)]() mutable {});
62+
}
63+
}
64+
5165
const moxygen::FetchOk& fetchOk() const override { return inner_->fetchOk(); }
5266

5367
void fetchCancel() override {
@@ -77,6 +91,13 @@ class CrossExecNamespacePublishHandle : public moxygen::Publisher::NamespacePubl
7791
)
7892
: inner_(std::move(inner)), exec_(std::move(exec)) {}
7993

94+
~CrossExecNamespacePublishHandle() override {
95+
// Inner dtor may touch session state; destroy it on exec_, not the dropping thread.
96+
if (inner_) {
97+
exec_->add([inner = std::move(inner_)]() mutable {});
98+
}
99+
}
100+
80101
void namespaceMsg(const moxygen::TrackNamespace& ns) override {
81102
exec_->add([inner = inner_, ns]() mutable { inner->namespaceMsg(ns); });
82103
}
@@ -98,6 +119,13 @@ class CrossExecSubscribeNamespaceHandle : public moxygen::Publisher::SubscribeNa
98119
)
99120
: inner_(std::move(inner)), exec_(exec) {}
100121

122+
~CrossExecSubscribeNamespaceHandle() override {
123+
// Inner dtor may touch session state; destroy it on exec_, not the dropping thread.
124+
if (inner_) {
125+
exec_->add([inner = std::move(inner_)]() mutable {});
126+
}
127+
}
128+
101129
const moxygen::SubscribeNamespaceOk& subscribeNamespaceOk() const override {
102130
return inner_->subscribeNamespaceOk();
103131
}
@@ -128,6 +156,13 @@ class CrossExecPublishBlockedHandle : public moxygen::Publisher::PublishBlockedH
128156
)
129157
: inner_(std::move(inner)), exec_(std::move(exec)) {}
130158

159+
~CrossExecPublishBlockedHandle() override {
160+
// Inner dtor may touch session state; destroy it on exec_, not the dropping thread.
161+
if (inner_) {
162+
exec_->add([inner = std::move(inner_)]() mutable {});
163+
}
164+
}
165+
131166
void publishBlocked(
132167
const moxygen::TrackNamespace& trackNamespaceSuffix,
133168
const std::string& trackName
@@ -150,6 +185,13 @@ class CrossExecSubscribeTracksHandle : public moxygen::Publisher::SubscribeTrack
150185
)
151186
: inner_(std::move(inner)), exec_(exec) {}
152187

188+
~CrossExecSubscribeTracksHandle() override {
189+
// Inner dtor may touch session state; destroy it on exec_, not the dropping thread.
190+
if (inner_) {
191+
exec_->add([inner = std::move(inner_)]() mutable {});
192+
}
193+
}
194+
153195
const moxygen::SubscribeTracksOk& subscribeTracksOk() const override {
154196
return inner_->subscribeTracksOk();
155197
}

test/MoqxRelaySubNsTests.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@ TEST_P(MoQRelayTest, ExactNamespaceSubscriberReceivesPublishNamespace) {
167167

168168
removeSession(publisher);
169169
removeSession(subscriber);
170+
driveIfMultiThread(); // flush exec-hopped handle destruction before mocks are destroyed
170171
}
171172

172173
// Bug: when a subscriber with forward=true joins a namespace whose track

0 commit comments

Comments
 (0)