-
Couldn't load subscription status.
- Fork 2.3k
Never give up in socket transport #7616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! I've reviewed the changes and found several issues that need attention before this can be merged.
|
|
||
| private readonly retryConfig: RetryConfig = { | ||
| maxInitialAttempts: 10, | ||
| maxInitialAttempts: Infinity, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is setting this to Infinity intentional? The while loop in connectWithRetry() will never exit naturally now, which could lead to resource exhaustion if the connection continuously fails. Consider:
- Making this configurable through the constructor's
retryConfigparameter - Adding a mechanism to cancel retry attempts (e.g., an abort signal)
- Implementing exponential backoff with a maximum retry duration instead of infinite attempts
| } catch (error) { | ||
| console.error( | ||
| `[SocketTransport] Initial connection attempts failed: ${error instanceof Error ? error.message : String(error)}`, | ||
| `[SocketTransport] Unexpected error in connection loop: ${error instanceof Error ? error.message : String(error)}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message might be misleading now. With infinite retries, errors in the connection loop aren't necessarily "unexpected" anymore. Could we update this to be more descriptive, like "Error in connection retry loop" or "Connection retry loop terminated unexpectedly"?
| console.log( | ||
| `[SocketTransport] Connection attempt ${this.retryAttempt + 1} / ${this.retryConfig.maxInitialAttempts}`, | ||
| ) | ||
| console.log(`[SocketTransport] Connection attempt ${this.retryAttempt + 1}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With maxInitialAttempts now set to Infinity, this while condition will always be true. Should we add an additional exit condition here, such as checking for a cancellation signal or a maximum retry duration?
Also, the removed code that threw an error after max attempts provided a clear exit condition. Without it, the only way to exit this loop is through a successful connection or an unexpected error.
|
|
||
| // Socket.IO has exhausted its reconnection attempts | ||
| // The connection is now permanently failed until manual intervention | ||
| this.connectionState = ConnectionState.RETRYING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is setting the state to RETRYING here intentional? Socket.IO has already exhausted its reconnection attempts at this point. This might confuse consumers checking the connection state, as it implies retrying is still happening when it's not. Should this remain as FAILED or perhaps a new state like AWAITING_MANUAL_RECONNECT?
Important
Allow infinite retry attempts in
SocketTransportby settingmaxInitialAttemptstoInfinityand adjusting connection state handling.maxInitialAttemptstoInfinityinSocketTransportto allow infinite retry attempts.connectionStatetoFAILEDafter exhausting retry attempts.connectWithRetry().reconnect_failedevent to setconnectionStatetoRETRYINGinstead ofFAILED.This description was created by
for 989fb2e. You can customize this summary. It will automatically update as commits are pushed.