Skip to content

Commit 1f8d73c

Browse files
afrindclaude
andcommitted
relay: give LocalForwarderRegistry a three-state entry
The registry mapped a track name straight to a forwarder, which let a reader attach to one before its largest and extensions arrived from upstream. A client reads that as a track restart. An entry is now absent, pending, or ready, and a pending entry does not hand out its forwarder. Only the Claim that join() returns can reach it, and dropping that Claim fails the entry instead of stranding whoever waits on it. Nothing observes the pending state yet, because both callers mark the entry ready on the next line; holding a claim open across setup is what the commits above this one do. LocalForwarderRegistryTest is new, and covers the three states, the identity checks, and the two asserts a caller can trip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent c95e54c commit 1f8d73c

5 files changed

Lines changed: 647 additions & 67 deletions

File tree

src/MoqxRelay.cpp

Lines changed: 29 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -459,8 +459,8 @@ MoqxRelay::validatePublishNamespace(const FullTrackName& ftn, RequestID requestI
459459
// tlForwarders_ must already be initialized.
460460
std::shared_ptr<MoQForwarder> MoqxRelay::createPublisherForwarder(const PublishRequest& pub) {
461461
const auto& ftn = pub.fullTrackName;
462-
auto localPubFwd = std::make_shared<MoQForwarder>(ftn, pub.largest);
463-
localPubFwd->setExtensions(pub.extensions);
462+
// Returned uninitialized; the caller's Claim applies the initial state.
463+
auto localPubFwd = std::make_shared<MoQForwarder>(ftn);
464464

465465
// removeOnEmpty=false: the publisher's forwarder must survive subscriber churn, so
466466
// LocalForwarderCallback removes it from tlForwarders_ only when the source ends.
@@ -497,10 +497,11 @@ Subscriber::PublishResult MoqxRelay::publishFromPublisherExec(
497497

498498
auto localPubFwd = createPublisherForwarder(pub);
499499

500-
// The publisher's forwarder is authoritative — claim the slot, displacing any
500+
// The publisher's forwarder is authoritative — claim the entry, displacing any
501501
// stale subscribe-path local forwarder so same-thread subscribers reuse THIS
502502
// forwarder via the fast path.
503-
tlForwarders_->set(pub.fullTrackName, localPubFwd);
503+
tlForwarders_->replace(pub.fullTrackName, localPubFwd)
504+
.markReady(InitialTrackState{pub.largest, pub.extensions});
504505

505506
// crossExecFilter is a channel subscriber for the relay exec
506507
// regulsterPublishOnRelay exec completes wiring the chain (topNFilter → terminationFilter →
@@ -1125,7 +1126,7 @@ folly::coro::Task<void> MoqxRelay::addSubscriberAndPublishViaLocalForwarder(
11251126

11261127
// Fast path: local forwarder already exists on this thread.
11271128
if (auto* localReg = tlForwarders_.get()) {
1128-
if (auto localFwd = localReg->get(ftn)) {
1129+
if (auto localFwd = localReg->getIfReady(ftn)) {
11291130
auto p = startPublish(subscriberSession, localFwd, forward, pinned, nullptr);
11301131
if (p) {
11311132
co_await awaitPublishReply(localFwd, std::move(p->subscriber), std::move(p->reply));
@@ -1145,11 +1146,7 @@ folly::coro::Task<void> MoqxRelay::addSubscriberAndPublishViaLocalForwarder(
11451146
}()
11461147
);
11471148

1148-
auto [localFwd, isNew, localReg] = acquireLocalForwarder(ftn, [&] {
1149-
auto fwd = std::make_shared<MoQForwarder>(ftn);
1150-
initial.applyTo(*fwd);
1151-
return fwd;
1152-
});
1149+
auto [localFwd, isNew, localReg] = acquireLocalForwarder(ftn, initial);
11531150

11541151
auto p = startPublish(subscriberSession, localFwd, forward, pinned, nullptr);
11551152
if (!p) {
@@ -1201,19 +1198,25 @@ folly::coro::Task<void> MoqxRelay::addSubscriberAndPublishViaLocalForwarder(
12011198
co_await awaitPublishReply(localFwd, std::move(p->subscriber), std::move(p->reply));
12021199
}
12031200

1204-
// Slow-path local-forwarder bootstrap shared by the publish and subscribe LF paths:
1205-
// ensures the thread-local registry, then getOrCreates the forwarder for ftn. Callers
1206-
// handle the fast path and install the PendingForwarderCallback (timing differs).
1207-
MoqxRelay::LocalForwarderBootstrap MoqxRelay::acquireLocalForwarder(
1208-
const FullTrackName& ftn,
1209-
folly::FunctionRef<std::shared_ptr<MoQForwarder>()> factory
1210-
) {
1201+
// Slow-path local-forwarder bootstrap shared by the publish and subscribe LF paths.
1202+
// Callers handle the fast path and install the PendingForwarderCallback themselves,
1203+
// because the timing differs.
1204+
MoqxRelay::LocalForwarderBootstrap
1205+
MoqxRelay::acquireLocalForwarder(const FullTrackName& ftn, const InitialTrackState& initial) {
12111206
if (!tlForwarders_.get()) {
12121207
tlForwarders_.reset(new LocalForwarderRegistry());
12131208
}
12141209
auto* localReg = tlForwarders_.get();
1215-
auto [localFwd, isNew] = localReg->getOrCreate(ftn, factory);
1216-
return {std::move(localFwd), isNew, localReg};
1210+
auto joined = localReg->join(ftn, [&] { return std::make_shared<MoQForwarder>(ftn); });
1211+
if (auto* claim = std::get_if<LocalForwarderRegistry::Claim>(&joined)) {
1212+
auto localFwd = claim->forwarder();
1213+
claim->markReady(initial);
1214+
return {std::move(localFwd), /*isNew=*/true, localReg};
1215+
}
1216+
auto* ready = std::get_if<LocalForwarderRegistry::Ready>(&joined);
1217+
XCHECK(ready) << "local forwarder entry still pending; a caller failed to resolve its claim: "
1218+
<< ftn;
1219+
return {ready->forwarder, /*isNew=*/false, localReg};
12171220
}
12181221

12191222
class MoqxRelay::NamespaceSubscription : public Publisher::SubscribeNamespaceHandle {
@@ -1961,10 +1964,9 @@ folly::coro::Task<Publisher::SubscribeResult> MoqxRelay::subscribeFromSubscriber
19611964
) {
19621965
const auto& ftn = subReq.fullTrackName;
19631966

1964-
// getOrCreate before the relay hop: serializes same-iothread races. isNew=false means
1965-
// the forwarder already exists (or a setup is in progress) on this thread — just attach.
1966-
auto [localFwd, isNew, localReg] =
1967-
acquireLocalForwarder(ftn, [&] { return std::make_shared<MoQForwarder>(ftn); });
1967+
// Join before the relay hop: serializes same-iothread races. No initial state yet —
1968+
// the upstream OK has not arrived, so an attacher can read a too-early largest here.
1969+
auto [localFwd, isNew, localReg] = acquireLocalForwarder(ftn, InitialTrackState{});
19681970

19691971
consumer =
19701972
wrapWithTrackStats(trackStats_, ftn, std::move(consumer), stats::TrackDirection::Egress);
@@ -2176,7 +2178,7 @@ MoqxRelay::trackStatusOnSubscriberExec(const TrackStatus& req) {
21762178
return std::nullopt;
21772179
}
21782180
auto* localReg = tlForwarders_.get();
2179-
auto localFwd = localReg ? localReg->get(req.fullTrackName) : nullptr;
2181+
auto localFwd = localReg ? localReg->getIfReady(req.fullTrackName) : nullptr;
21802182
if (!localFwd || localFwd->numForwardingSubscribers() == 0) {
21812183
return std::nullopt;
21822184
}
@@ -2190,7 +2192,7 @@ Fetch MoqxRelay::fetchOnSubscriberExec(Fetch fetch, const std::shared_ptr<MoQSes
21902192
return fetch;
21912193
}
21922194
auto* localReg = tlForwarders_.get();
2193-
auto localFwd = localReg ? localReg->get(fetch.fullTrackName) : nullptr;
2195+
auto localFwd = localReg ? localReg->getIfReady(fetch.fullTrackName) : nullptr;
21942196
if (localFwd) {
21952197
auto res = localFwd->resolveJoiningFetch(session, *joining);
21962198
if (res.hasValue()) {
@@ -2272,7 +2274,7 @@ MoqxRelay::fetchImpl(Fetch fetch, std::shared_ptr<FetchConsumer> consumer) {
22722274
folly::coro::Task<std::optional<TrackStatusOk>>
22732275
MoqxRelay::readPublisherForwarderStatus(bool hasHandle, TrackStatus req) {
22742276
auto* localReg = tlForwarders_.get();
2275-
auto forwarder = localReg ? localReg->get(req.fullTrackName) : nullptr;
2277+
auto forwarder = localReg ? localReg->getIfReady(req.fullTrackName) : nullptr;
22762278
if (!forwarder || forwarder->numForwardingSubscribers() == 0) {
22772279
co_return std::nullopt;
22782280
}
@@ -2591,7 +2593,7 @@ void MoqxRelay::onTrackEvicted(const FullTrackName& ftn, std::shared_ptr<MoQSess
25912593
// publisher forwarder; evict it on its owning exec.
25922594
folly::via(session->getExecutor(), [this, ftn, evict = std::move(evict)]() {
25932595
auto* localReg = tlForwarders_.get();
2594-
evict(localReg ? localReg->get(ftn) : nullptr);
2596+
evict(localReg ? localReg->getForEviction(ftn) : nullptr);
25952597
});
25962598
return;
25972599
}

src/MoqxRelay.h

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -338,10 +338,8 @@ class MoqxRelay : public moxygen::Publisher,
338338
bool isNew{false};
339339
LocalForwarderRegistry* localReg{nullptr};
340340
};
341-
LocalForwarderBootstrap acquireLocalForwarder(
342-
const moxygen::FullTrackName& ftn,
343-
folly::FunctionRef<std::shared_ptr<moxygen::MoQForwarder>()> factory
344-
);
341+
LocalForwarderBootstrap
342+
acquireLocalForwarder(const moxygen::FullTrackName& ftn, const InitialTrackState& initial);
345343

346344
bool addSubscriberAndPublish(
347345
std::shared_ptr<moxygen::MoQSession> subscriberSession,

0 commit comments

Comments
 (0)