-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix(api-client): socket reconnect after coming back from sleep #6287
Conversation
if (this.socket) { | ||
this.logger.info(`Disconnecting from WebSocket (reason: "${reason}")`); | ||
// TODO: 'any' can be removed once this issue is resolved: | ||
// https://github.com/pladaria/reconnecting-websocket/issues/44 | ||
(this.socket as any).close(CloseEventCode.NORMAL_CLOSURE, reason, { |
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 method does not accept the 3rd (options) parameter any more.
Quality Gate passedIssues Measures |
} | ||
|
||
if (wasAsleep) { | ||
if (!isDisconnectedCallback || wasDisconnected) { |
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.
Shouldn't be like that? 🤔
if (!isDisconnectedCallback || wasDisconnected) { | |
if (!isDisconnectedCallback?.() || wasDisconnected) { |
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.
No, if the check callback is not provided at all we want to always run the callback
onBackFromSleep({ | ||
callback: () => { | ||
if (this.socket) { | ||
this.logger.debug('Back from sleep, reconnecting WebSocket'); | ||
this.socket?.reconnect(); | ||
} | ||
}, | ||
isDisconnected: () => this.getState() === WEBSOCKET_STATE.CLOSED, |
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.
Wasn't the ping/pong mechanism enough to detect that the connection was closed?
Is there some kind of timer that runs to check if a message has been received in a certain amount of time?
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.
Yeah, we have the mechanism for that, but it's a ping every 20 seconds, which doesn't seem to be efficient enough to reconnect immediately after coming back from sleep. From what I was testing, it was the 20s that we had to wait for the weboscket to reconnect after coming back from sleep.
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.
Humm that makes sense, 20s is, indeed, pretty long time.
Then your solution seems solid to me 😎
Once we've realised that the app was (probably) sleeping and during the possible "sleep time" the socket was disconnected, we will immediately reconnect. This should fix the bug where after coming back from sleep mode, messages are not synced immediately.