Skip to content

Frames arriving before StartMessageProcessor bypass the bounded queue #113

Description

@Platonenkov

Frames that arrive before StartMessageProcessor has run bypass the bounded queue, and the obvious fix is blocked by an unrelated coupling.

The window

OnceOpen ends like this:

try
{
    connectionManager.ResolveAllAwaiting();
    if (OnConnected is not null)
    {
        await OnConnected?.Invoke();      // consumer subscribes here
    }
    ...
}
...
StartPingTimer();
StartMessageProcessor();                  // channel created only now

Subscribing from an OnConnected handler is the normal pattern — it is what the wallet consuming this SDK does. The node can answer that subscription before the handler returns, so the first frames find _streamMessageChannel still null and take the fallback in EnqueueStreamMessage:

else
{
    _ = ProcessStreamMessageFireAndForgetAsync(frame);
}

Those frames are outside everything the queue provides: StreamMessageQueueCapacity does not apply to them, DroppedStreamMessages does not count them, and they are dispatched concurrently rather than one at a time — so the first events after connecting are exactly the ones that can arrive out of order.

Why the one-line fix does not work

Moving StartMessageProcessor() ahead of the try looks right and fails, measurably: TestUDroppedStreamMessagesCountsWhatTheConsumerNeverSaw goes from 10 evictions to 0.

The reason is a coupling that has nothing to do with message processing:

private void StartPingTimer()
{
    ...
    StopPingTimerSync();     // and this calls StopMessageProcessor()

So starting the processor before StartPingTimer has it torn down again moments later, and the fallback path handles every frame from then on. Stopping a ping timer should not stop the message queue; that is the actual defect behind this one.

What to do

  1. Remove StopMessageProcessor() from StopPingTimerSync, so the two lifecycles are independent.
  2. Add explicit StopMessageProcessor() calls where stopping is genuinely intended — Disconnect, DisconnectAndWaitAsync, OnceClose, OnConnectHandlerFailedAsync, and the ChangeServer / RetireCurrentSessionAndReconnectAsync paths, which currently rely on the side effect.
  3. Then move StartMessageProcessor() ahead of the OnConnected callback, so the channel exists before consumer code can subscribe.
  4. Once frames can no longer arrive without a channel, the fire-and-forget fallback in EnqueueStreamMessage covers only the not-connected case — worth checking whether it can go entirely.

A test can pin this: subscribe from an OnConnected handler, feed frames from inside that handler, and assert DroppedStreamMessages accounts for them under a small StreamMessageQueueCapacity — impossible today, since they never reach the queue.

Found by review of #111. Not introduced there: the ordering predates it, and the browser bypass removed in that PR was a separate path.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions