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:
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.
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,anyForwardedis set totrueBEFORE callingfn(). Iffn()causes an immediate soft error (tombstone),anyForwardedisalready true and the function returns SUCCESS, hiding the fact that all consumers
rejected the call.
Fix: rename to
anySuccess, move the set to AFTERfn()returns, and only setit when the subgroup consumer is still non-null:
Same fix in the new-consumer branch (after emplacing and calling fn).
Effect:
forEachSubscriberSubgroupreturns CANCELLED on the firstobject()call when all subscribers immediately tombstone. This correctly signals the
publisher to stop.
Change 2: don't cleanupOnError for non-terminal operations
cleanupOnErrorerases the subgroup fromsubgroups_when an error isreturned. When all subscribers tombstone (soft error), this removes the subgroup
from the map. A subsequent duplicate
beginSubgroupthen finds nothing andcreates a new SubgroupForwarder — returning SUCCESS instead of CANCELLED.
Fix: In
object()(non-finSubgroup path),beginObject(), andobjectPayload()(non-finSubgroup path), don't call
cleanupOnError— just returnresdirectly.Only terminal operations (
endOfSubgroup,finSubgroup=true,reset) shouldcall
removeSubgroupAndCheckEmpty().Effect: When object() returns CANCELLED (all tombstoned), the subgroup stays in
subgroups_. A duplicatebeginSubgroupfinds it, sees only tombstonedconsumers (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 subscriberstombstone on the first call. Is this the right signal?
Yes. no point sending more data; a future
requestUpdate fwd=1can signal renewed interest; it's the honest answer.