Skip to content

Don't reference ConnectAsync's CancellationToken while waiting for the first message#991

Merged
mayuki merged 1 commit intomainfrom
feature/FixStreamingHubCancellationHandling
Dec 2, 2025
Merged

Don't reference ConnectAsync's CancellationToken while waiting for the first message#991
mayuki merged 1 commit intomainfrom
feature/FixStreamingHubCancellationHandling

Conversation

@mayuki
Copy link
Member

@mayuki mayuki commented Dec 2, 2025

This PR fixes the handling of the CancellationToken when clients connect.

It resolves an issue where canceling the CancellationToken passed to ConnectAsync immediately after calling ConnectAsync, before the first message arrives, would cancel message reading.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a critical issue with CancellationToken handling in StreamingHub client connections. The change ensures that canceling the CancellationToken passed to ConnectAsync after the connection is established (but before the first message arrives) no longer incorrectly cancels the message reading loop.

Key Changes:

  • Modified reader.MoveNext to use only subscriptionToken instead of a linked token that includes connectAndSubscribeCancellationToken
  • Improved error message clarity for server version negotiation failures
  • Added comprehensive test coverage for the cancellation scenario

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
src/MagicOnion.Client/StreamingHubClientBase.cs Removed connectAndSubscribeCancellationToken from the first MoveNext call to prevent premature cancellation of message reading; improved error message for missing server version
tests/MagicOnion.Client.Tests/StreamingHubTest.cs Added test ConnectAsync_CancellationToken_Timeout_On_FirstMoveNext to verify canceling the connect token after connection doesn't affect message processing; added explicit using System.Threading directive

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

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