Skip to content

Fix null ref exception when websocket client retries. - #4589

Open
TingluoHuang wants to merge 2 commits into
mainfrom
users/tihuang/fixwebsocket
Open

Fix null ref exception when websocket client retries.#4589
TingluoHuang wants to merge 2 commits into
mainfrom
users/tihuang/fixwebsocket

Conversation

@TingluoHuang

Copy link
Copy Markdown
Member

This pull request refactors how the websocket client is initialized and managed in ResultsServer.cs, focusing on improving code clarity and reliability. The main change is extracting websocket client creation logic into a new CreateWebSocketClient method, which reduces code duplication and centralizes header setup. The refactor also ensures that the websocket client is properly disposed and re-initialized on failures.

WebSocket Client Initialization and Management:

  • Extracted websocket client creation and header setup into a new private method CreateWebSocketClient, replacing inline instantiation and header assignment logic. [1] [2]
  • Updated all calls to InitializeWebsocketClient to remove the explicit accessToken parameter, using the instance _token instead for authorization. [1] [2] [3] [4]
  • Ensured the websocket client is disposed and set to null before re-initialization after connection failures, preventing resource leaks and ensuring a fresh client is used. [1] [2]
  • Modified ConnectWebSocketClient to re-create the websocket client if it's null before attempting a connection, improving robustness in reconnection scenarios.

These changes improve maintainability, reduce duplication, and enhance the stability of websocket connections.

@TingluoHuang
TingluoHuang requested a review from a team as a code owner July 29, 2026 20:58
Copilot AI review requested due to automatic review settings July 29, 2026 20:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Refactors ResultServer’s websocket initialization/retry flow to avoid null-reference failures during reconnect attempts by centralizing ClientWebSocket creation and improving retry robustness.

Changes:

  • Extracted websocket client creation (headers/user-agent setup) into CreateWebSocketClient.
  • Updated websocket initialization to rely on the instance _token rather than passing an accessToken parameter.
  • Added additional disposal/re-init behavior on failures and ensured connect attempts recreate the client when needed.
Show a summary per file
File Description
src/Runner.Common/ResultsServer.cs Refactors websocket client creation and adjusts reconnect/disposal behavior to address retry-time null reference exceptions.

Review details

Comments suppressed due to low confidence (1)

src/Runner.Common/ResultsServer.cs:275

  • In the !delivered "Giving up" path you dispose/null _websocketClient, but any in-flight _websocketConnectTask started by the retry loop (e.g., after InitializeWebsocketClient(_liveConsoleFeedUrl, delay)) is not canceled/awaited here. Since ConnectWebSocketClient can (re)create a client when it observes _websocketClient == null, a background connect attempt can still re-establish the connection after we "give up", contradicting the intent of this block and creating disposal/connect races. Consider introducing a dedicated CancellationTokenSource for connect attempts (cancel it here and before starting a new connect), or otherwise ensuring no connect attempt can proceed after this block runs.
            if (!delivered)
            {
                // Giving up for now, so next invocation of this method won't attempt to reconnect
                _websocketClient?.Dispose();
                _websocketClient = null;
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread src/Runner.Common/ResultsServer.cs
if (!string.IsNullOrEmpty(liveConsoleFeedUrl))
{
_liveConsoleFeedUrl = liveConsoleFeedUrl;
InitializeWebsocketClient(liveConsoleFeedUrl, token, TimeSpan.Zero, retryConnection: true);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

we have private string _token; defined in the class, so we can just use that and not pass anything around.

Trace.Info($"Attempting to start websocket client with delay {delay}.");
await Task.Delay(delay);
using var connectTimeoutTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
await this._websocketClient.ConnectAsync(new Uri(feedStreamUrl), connectTimeoutTokenSource.Token);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

after the catch, _websocketClient become null.

@salmanmkc salmanmkc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

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.

3 participants