facebookexperimental / facebookexperimental/moxygen

Problems with MoQForwarder Tombstoning

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

Description

## Summary of the bug

Two related issues in tombstone handling that together cause incorrect behavior
when all downstream subscribers STOP_SENDING a subgroup, and the publisher then
opens a duplicate beginSubgroup for the same (group, subgroup):

### Change 1: anyForwarded → anySuccess

In `forEachSubscriberSubgroup`, `anyForwarded` is set to `true` BEFORE calling
`fn()`. If `fn()` causes an immediate soft error (tombstone), `anyForwarded` is
already true and the function returns SUCCESS, hiding the fact that all consumers
rejected the call.

Fix: rename to `anySuccess`, move the set to AFTER `fn()` returns, and only set
it when the subgroup consumer is still non-null:

```cpp
fn(sub, subgroupConsumerIt->second);
if (subgroupConsumerIt->second) { // not tombstoned by fn
anySuccess = true;
}
```

Same fix in the new-consumer branch (after emplacing and calling fn).

Effect: `forEachSubscriberSubgroup` returns CANCELLED on the first `object()`
call when all subscribers immediately tombstone. This correctly signals the
publisher to stop.

### Change 2: don't cleanupOnError for non-terminal operations

`cleanupOnError` erases the subgroup from `subgroups_` when an error is
returned. When all subscribers tombstone (soft error), this removes the subgroup
from the map. A subsequent duplicate `beginSubgroup` then finds nothing and
creates a new SubgroupForwarder — returning SUCCESS instead of CANCELLED.

Fix: In `object()` (non-finSubgroup path), `beginObject()`, and `objectPayload()`
(non-finSubgroup path), don't call `cleanupOnError` — just return `res` directly.
Only terminal operations (`endOfSubgroup`, `finSubgroup=true`, `reset`) should
call `removeSubgroupAndCheckEmpty()`.

Effect: When object() returns CANCELLED (all tombstoned), the subgroup stays in
`subgroups_`. A duplicate `beginSubgroup` finds it, sees only tombstoned
consumers (anyReset=false), detaches the old SubgroupForwarder, and returns
CANCELLED.

### Why both are needed

- Change 1 alone: object() returns CANCELLED on first call, but cleanupOnError
still erases the subgroup — duplicate beginSubgroup creates new forwarder,
returns SUCCESS.

- Change 2 alone: subgroup stays, but anyForwarded fires before fn errors, so
object(0) returns SUCCESS. The loop needs two iterations before CANCELLED.
The duplicate beginSubgroup scenario only works if the publisher calls object()
twice first.

## Open design question: return SUCCESS or CANCELLED when all tombstoned?

With both changes, `object()` returns CANCELLED immediately when all subscribers
tombstone on the first call. Is this the right signal?

Yes. no point sending more data; a future `requestUpdate fwd=1`
can signal renewed interest; it's the honest answer.

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.