Respect CancellationToken in HubConnection.StartAsync() #10140
Conversation
… to network operations: both to NegotiateAsync(), and to StartTransport(). ITransport.StartAsync() modified to accept a CancellationToken.
…d to ITransport.StartAsync().
danielloczi
requested review from
analogrelay,
BrennanConroy,
halter73 and
mikaelm12
as code owners
|
|
src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/ITransport.cs
Outdated
Show resolved
Hide resolved
|
@danielloczi Can you:
|
|
Yes, will be there within a few mins! |
…ing CancellationToken.None was removed from unit tests of all 3 transports (files were reverted to their original state), and the change was followed up in TestTransport.cs and EndToEndTests.cs.
|
@davidfowl CancellationToken.None was removed from all unit tests (they were reverted to their original versions). We're using the "Cancellation cancellationToken = default" syntax in ITransport.StartAsync() and all locations where this interface is implemented (the 3 transports and the 2 test files). |
Yep, we generally try to merge commit external PRs. We ❤️ having external contributors in the commit history! (Reminder to @BrennanConroy and @halter73 who will probably be merging this ;P) |
|
Is there anything we can do to fix that failing test? |
|
It's not related to the change :). We have some flaky tests and you just got hit with them. I'll handle it and restart the pipeline. |
|
This comment was made automatically. If there is a problem contact [email protected]. I've triaged the above build. I've created/commented on the following issue(s) |
|
This comment was made automatically. If there is a problem contact [email protected]. I've triaged the above build. I've created/commented on the following issue(s) |
src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/ITransport.cs
Show resolved
Hide resolved
|
@danielloczi I think we can merge this after #10140 (comment) is addressed. |
… token param of StartAsync(). StopAsync() now can short circuit the start/reconnect attempt faster.
…gether by StartAsyncInner(). StartAsyncCore() and HandshakeAsync() methods not linking stopCts again.
|
Here's an issue to track adding tests: #10432 |
halter73
changed the title
|
Thanks @danielloczi! |
|
It was a team work together with @BalintFarkas! Thanks! |
|
@anurse @halter73 If I see correctly, our commits were squashed after all, so I was removed from the list of authors :( Is there some way to add them un-squashed, or somehow add me as author, too? Just for the sweet glory :) Thanks!! Edit: alternatively, I'd love to help out by creating the unit tests for this scenario, as tracked in this issue: #10432, and get the coveted contributorship that way. Would that be fine? |
|
Sorry about that @BalintFarkas . We generally merge external contributions but it looked like this got squashed. I don't think it's really possible to unsquash...
That would certainly be OK! |
|
Sorry @BalintFarkas! Totally my fault. I'll try not to let it happen it again. |
|
No problem - I'll get to work on the unit tests! :) |
Summary of the changes:
HubConnection.StartAsync() can be cancelled now even after network operations have started. CancellationToken passed through to HttpConnection and transports (LongPolling, ServerSentEvents, WebSocket).
Addresses #10059
If possible, please don't squash the commits of the PR so that both our names remain in the commit history :)