Skip to content

Deliver queued notifications in the order they were received - #1162

Draft
dwcullop wants to merge 1 commit into
reactivemarbles:mainfrom
dwcullop:fix/delivery-queue-ordering
Draft

Deliver queued notifications in the order they were received#1162
dwcullop wants to merge 1 commit into
reactivemarbles:mainfrom
dwcullop:fix/delivery-queue-ordering

Conversation

@dwcullop

@dwcullop dwcullop commented Aug 5, 2026

Copy link
Copy Markdown
Member

SharedDeliveryQueue does not deliver in the order it received.

receipt  : A1, A2, Bx
delivered: A1, Bx, A2

Pending notifications are partitioned into a typed sub-queue per source, and DrainPending picked the next one with Bitset.FindHighest(). That selects by sub-queue index, not by arrival.

Reachable through the operators, not just by driving the queue directly. SynchronizeSafe(queue) creates a sub-queue per subscription, so in Page the page requests and the data changes land in different sub-queues and both mutate the same Paginator.

Change

- var sourceIndex = _activeBits.FindHighest();
- var active = _sources[sourceIndex];
+ var source = _order.Dequeue();

_order is a Queue<DrainableBase> holding one entry per enqueued notification, recording which sub-queue it came from. The payloads stay where they are:

_items.Enqueue(item);         // typed, struct, no allocation
_parent.EnqueueOrder(this);   // a reference that already exists

That part matters. The obvious alternative is one queue of type-erased notifications, but Notification<T> is a readonly struct, so queuing one allocates nothing today. Recording only the order keeps it that way.

Selecting by arrival leaves Bitset, FindHighest, SetActive, NotifyQueueRemoved, CompactIfNeeded, _sources and the per-sub-queue Index all dead. Bitset had no other caller.

Highest-index-first was deliberate

Newer sub-queues are children of older ones, and a child had to drain before a parent's delivery could dispose it. Arrival ordering drops that guarantee, so a disposed sub-queue's stale order entries get skipped when the drain reaches them instead. Third test covers it.

Tests

Two of the three fail on main:

Expected {"int:1","int:2","str:hello"}, but {"int:1","str:hello","int:2"} differs at index 1
Expected {"int:0","str:a","int:2","str:b","int:4"}, but {"int:0","str:a","str:b","int:2","int:4"} differs at index 2

The bug only bites when the queue holds more than one item, which needs cross-thread contention. Single threaded the reentrant path drains inline, so nothing is queued to reorder. That is why the suite passes without this.

Related

#1163 moves change batching out of the queue. Independent, but that batching was masking this one: everything in a drain cycle got folded into a single changeset before anything downstream saw the order. Same file, so whichever lands second needs a trivial merge.

SharedDeliveryQueue partitions pending notifications into a typed sub-queue
per source, and the drain loop picked the next one with
Bitset.FindHighest(). Selection was therefore by sub-queue index rather than
by arrival, so notifications from two sources could be delivered in an order
they were not received in. With receipt order A1, A2, Bx, delivery came out
A1, Bx, A2.

The sub-queues are still typed, so the payloads are still held as structs
and queuing one still costs no allocation. What changes is that a second
queue now records which source each pending notification came from, in
arrival order, and the drain loop follows that instead of scanning a bitset.
The entries are sub-queue references that already exist, so recording the
order does not allocate either.

Highest-index-first was there so a child sub-queue drained before a parent's
delivery could dispose it. That is no longer needed. A disposed sub-queue
drops its pending notifications, and its leftover order entries are skipped
when the drain reaches them, which is the same outcome by a shorter route.

This also removes Bitset, which had no other caller, along with the
sub-queue index bookkeeping and the compaction pass that existed to keep the
bitset dense.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9582bb33-26d3-4aa5-8dd7-57dc55304680
@dwcullop dwcullop changed the title Deliver queued notifications in the order they were received Make the delivery queue deliver in order, and only serialize Aug 5, 2026
@dwcullop
dwcullop force-pushed the fix/delivery-queue-ordering branch from 273fb40 to b07c03c Compare August 6, 2026 02:09
@dwcullop dwcullop changed the title Make the delivery queue deliver in order, and only serialize Deliver queued notifications in the order they were received Aug 6, 2026

@JakenVeina JakenVeina left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as the preservation of ordering goes, this looks right to me.

Feel free to ignore the bit about AggressiveInlining for now, as it's already littered throughout the rest of the class. But I figured I'd mention it.

public DeliverySubQueue<T> CreateQueue<T>(IObserver<T> observer) => new(this, observer);

/// <summary>Acquires the gate for read-only inspection. Does not trigger delivery on dispose.</summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per official docs, don't use AggressiveInlining unless you can back it up with benchmarks to prove it's actually beneficial.

Adding AggressiveInlining is probably one of a variety of optimizations worth exploring, once the API is settled and proven in the field, especially since this whole class is basically a hot path. But I agree with the docs, it's premature. Plus, the runtime is QUITE good at applying optimizations like inlining on hot paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants