facebookexperimental / facebookexperimental/moxygen

Relay SUBSCRIBE_NAMESPACE handling makes redundant REQUEST_UPDATE

Open
#145 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
111
Forks
35
Avg merge
4m
Merged PRs (30d)
1

Description

Bug 1 — Double REQUEST_UPDATE when forwarder was empty (the main one)

When forwarder->empty() and subNs.forward=true:
- Line 864 fires doSubscribeUpdate(handle, true) async
- Then publishToSession → forwarder->addSubscriber(..., true) → forwardChanged fires (promise is already fulfilled since this is an
existing subscription) → fires doSubscribeUpdate(handle, true) again

Two REQUEST_UPDATEs race to upstream for what should be one.

Bug 2 — Redundant REQUEST_UPDATE when subNs.forward=false

If forwarder->empty() but subNs.forward=false, upstream is already at forward=false (onEmpty sent it). Line 864 fires
REQUEST_UPDATE(false) for no reason.

Bug 3 — The entire line 860-864 block is unnecessary

forwardChanged already correctly handles the 0→1 forwarding transition for any non-empty-forwarder case (all-false subscribers + new true
subscriber). So the explicit check at 860 isn't filling a gap — it's just causing doubles.

The fix is to delete lines 860-865 entirely. forwardChanged handles all the cases correctly via numForwardingSubscribers() > 0.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.