Skip to content

fix: always use ConnectTimeout when connecting the WebSocket#27

Merged
deleteLater merged 3 commits intofeatbit:mainfrom
ghelyar:reconnect-timeout
Jun 20, 2025
Merged

fix: always use ConnectTimeout when connecting the WebSocket#27
deleteLater merged 3 commits intofeatbit:mainfrom
ghelyar:reconnect-timeout

Conversation

@ghelyar
Copy link
Copy Markdown
Contributor

@ghelyar ghelyar commented Jun 19, 2025

Change all connection attempts to use the configured ConnectTimeout, including reconnect attempts.

Fixes #26

@ghelyar
Copy link
Copy Markdown
Contributor Author

ghelyar commented Jun 19, 2025

@deleteLater please review

@deleteLater deleteLater requested a review from Copilot June 19, 2025 16:08
@deleteLater deleteLater added the bug Something isn't working label Jun 19, 2025
@deleteLater deleteLater moved this to In Progress in FeatBit Jun 19, 2025
@deleteLater deleteLater moved this from In Progress to PR Ready in FeatBit Jun 19, 2025
@deleteLater deleteLater self-requested a review June 19, 2025 16:09
Copy link
Copy Markdown
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 centralizes timeout handling by ensuring all WebSocket connection and close operations use the configured ConnectTimeout and CloseTimeout from FbOptions.

  • Inject FbOptions into WebSocketTransport and FbWebSocket
  • Update StartAsync signatures to drop ad-hoc timeout parameters and rely on options
  • Extend tests to cover connection timeout behavior

Reviewed Changes

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

Show a summary per file
File Description
tests/FeatBit.ServerSdk.Tests/Transport/WebSocketsTransportTests.cs Updated transport factory calls to accept FbOptions and added a timeout test
tests/FeatBit.ServerSdk.Tests/TestApp.cs Modified CreateWebSocketTransport to require FbOptions
src/FeatBit.ServerSdk/Transport/WebSocketTransport.cs Applied ConnectTimeout/CloseTimeout from options and removed inline parameters
src/FeatBit.ServerSdk/Transport/FbWebSocket.cs Refactored transport factory to accept FbOptions and reordered ConnectAsync parameters
src/FeatBit.ServerSdk/DataSynchronizer/WebSocketDataSynchronizer.cs Simplified StartAsync but removed its use of ConnectTimeout
Comments suppressed due to low confidence (2)

src/FeatBit.ServerSdk/DataSynchronizer/WebSocketDataSynchronizer.cs:67

  • The updated DataSynchronizer StartAsync no longer enforces a connect timeout; consider adding a unit test to verify that ConnectTimeout is honored in this code path.
        public Task<bool> StartAsync()

src/FeatBit.ServerSdk/Transport/FbWebSocket.cs:257

  • A positional argument follows a named argument here, which is invalid in C#. Change to ConnectAsync(isReconnecting: true, cancellationToken: _stopCts.Token) or reorder parameters.
                    await ConnectAsync(isReconnecting: true, _stopCts.Token).ConfigureAwait(false);

@deleteLater
Copy link
Copy Markdown
Contributor

I have made some nit changes and will test this PR tomorrow morning.

@deleteLater
Copy link
Copy Markdown
Contributor

LGTM! Thanks for the fix.

@deleteLater deleteLater merged commit 4f8af8b into featbit:main Jun 20, 2025
1 check passed
@github-project-automation github-project-automation bot moved this from PR Ready to Done in FeatBit Jun 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

bug: reconnecting does not use ConnectTimeout

3 participants