Skip to content

Conversation

BrennanConroy
Copy link
Member

Fixes #58947

There is a race between when the IHostApplicationLifetime.ApplicationStopping token fires and when the IServer.StopAsync (which unbinds connection listeners) runs (ref). SignalR was listening to ApplicationStopping and closing all currently active connections, however new incoming connections right after ApplicationStopping fires are still accepted by the IServer implementation and SignalR was processing them and letting them stay around until the IServer implementation ungracefully closed them (30 second default).

The first part of the fix was to add a lock around CloseConnections and then call CloseConnections when creating a new connection if we know that the application is stopping.

While testing to make sure this fix worked, I noticed that connections with stateful reconnect enabled weren't shutting down quickly, this was because when we first created the connection we immediately ran CancelPreviousPoll which would clear the cancellation token being used to stop the websocket read loop. The fix was to not run CancelPreviousPoll for the first connection and move where the CancellationToken is created to be after the previous connection is canceled.

@Copilot Copilot AI review requested due to automatic review settings October 17, 2025 22:42
@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Oct 17, 2025
Copy link
Contributor

@Copilot 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

Addresses race during application shutdown where new SignalR connections could still be accepted after ApplicationStopping, causing delayed teardown and lingering stateful reconnect sessions.

  • Adds shutdown-aware logic to HttpConnectionManager (tracking _closed, locking CloseConnections, early closing on create when stopping).
  • Adjusts WebSocket connection handling to defer creating CancellationTokenSource until needed and adds negotiate-time error when server is stopping.
  • Expands test coverage for new shutdown scenarios (negotiate after stop, stateful reconnect close behavior, creating connection after stop).

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/SignalR/common/Http.Connections/test/HttpConnectionManagerTests.cs Adds test ensuring new connections are immediately disposed after application stopping.
src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs Adds tests for negotiate failure post-stop and stateful reconnect shutdown behavior; overload for passing lifetime into manager.
src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs Introduces application lifetime awareness, lock, and closed state logic to prevent accepting new connections during shutdown.
src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs Adjusts cancellation token timing and adds negotiate error when a disposed connection is created during shutdown.
Comments suppressed due to low confidence (1)

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionManager.cs:1

  • Task.WaitAll executes a potentially long-running synchronous wait while holding _closeLock, increasing contention and risking blocked threads if disposal needs thread-pool scheduling. Collect the tasks inside the lock, then release the lock before awaiting (e.g., copy tasks to a local list, exit the lock, and use Task.WhenAll with a timeout via Task.WhenAny), or refactor CloseConnections to be async to avoid blocking while holding the lock.
// Licensed to the .NET Foundation under one or more agreements.

Comment on lines +29 to +31
private readonly IHostApplicationLifetime _applicationLifetime;
private readonly Lock _closeLock = new();
private bool _closed;
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

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

The field _closed reads like a verb; renaming to _isClosed (and updating references) improves clarity by conveying a boolean state.

Copilot generated this review using guidance from repository custom instructions.

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

Labels

area-signalr Includes: SignalR clients and servers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Signalr version 8.0.5 - If I have even one connected client, shutdown of the app is 30 seconds

1 participant