Skip to content

Simplify ManagedWebSocket.EnsureBufferContainsAsync#127470

Open
cittaz wants to merge 1 commit intodotnet:mainfrom
cittaz:cleanup/websocket-ensure-buffer-contains
Open

Simplify ManagedWebSocket.EnsureBufferContainsAsync#127470
cittaz wants to merge 1 commit intodotnet:mainfrom
cittaz:cleanup/websocket-ensure-buffer-contains

Conversation

@cittaz
Copy link
Copy Markdown
Contributor

@cittaz cittaz commented Apr 27, 2026

Summary

Cleans up ManagedWebSocket.EnsureBufferContainsAsync by removing a dead branch and a stale comment that have been left behind since #69272 replaced the manual read loop with a single Stream.ReadAtLeastAsync call. No behavior change.

Background

Before #69272 (May 2022), the method ended with a real loop:

// While we don't have enough data, read more.
while (_receiveBufferCount < minimumRequiredBytes)
{
    int numRead = await _stream.ReadAsync(_receiveBuffer.Slice(_receiveBufferCount), cancellationToken).ConfigureAwait(false);
    Debug.Assert(numRead >= 0, $"Expected non-negative bytes read, got {numRead}");
    if (numRead <= 0)
    {
        ThrowEOFUnexpected();
        break;
    }
    _receiveBufferCount += numRead;
}

That PR converted it to a single ReadAtLeastAsync call (which loops internally until enough bytes are read or EOF) and downgraded while to if, but kept both the inner predicate and the original comment.

What was wrong

if (_receiveBufferCount < minimumRequiredBytes)              // outer check
{
    if (_receiveBufferCount > 0)
    {
        _receiveBuffer.Span.Slice(_receiveBufferOffset, _receiveBufferCount).CopyTo(_receiveBuffer.Span);
    }
    _receiveBufferOffset = 0;

    // While we don't have enough data, read more.
    if (_receiveBufferCount < minimumRequiredBytes)          // dead: same condition, count unchanged
    {
        ...
    }
}
  • Dead branch. Between the outer check and the inner check, only CopyTo (which shifts data within the buffer) and _receiveBufferOffset = 0 execute. Neither mutates _receiveBufferCount, so the inner predicate is provably the same as the outer one. The body always runs.
  • Stale comment. // While we don't have enough data, read more. describes the original while loop. The current code performs a single ReadAtLeastAsync call (the loop is inside ReadAtLeastAsync itself), so "while" no longer matches.

Change

Remove the inner if, drop the misleading comment, and dedent the body. Behavior is unchanged.

The inner `if (_receiveBufferCount < minimumRequiredBytes)` was a fossil
left over from when this method had a manual `while` loop calling
`Stream.ReadAsync` until the buffer held enough data. After dotnet#69272
replaced that loop with a single `Stream.ReadAtLeastAsync` call (which
loops internally), the inner predicate became unreachable in any branch
where it could be false: nothing between the outer check and the inner
check mutates `_receiveBufferCount`. The accompanying `// While we don't
have enough data, read more.` comment also no longer matches the code.

Remove the dead branch, drop the stale comment, and dedent the body.
No behavior change.
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Apr 27, 2026
Copy link
Copy Markdown
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Thanks

@MihaZupan MihaZupan added this to the 11.0.0 milestone Apr 27, 2026
@MihaZupan MihaZupan enabled auto-merge (squash) April 27, 2026 21:07
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

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

Labels

area-System.Net community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants