-
Notifications
You must be signed in to change notification settings - Fork 40
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
refactor: [DRGO-1410]: ramin/refactor_ws_connection_management #351
Merged
ramin-deriv
merged 25 commits into
deriv-com:master
from
ramin-deriv:ramin/use_ready_flag
Dec 4, 2024
Merged
refactor: [DRGO-1410]: ramin/refactor_ws_connection_management #351
ramin-deriv
merged 25 commits into
deriv-com:master
from
ramin-deriv:ramin/use_ready_flag
Dec 4, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramin-deriv
changed the title
ramin/refactor_ws_connection_management
refactor: [DRGO-1410]: ramin/refactor_ws_connection_management
Nov 7, 2024
Comment on lines
122
to
126
unawaited(_webSocketChannel?.ready.then((_) { | ||
print('#### Timer started ${DateTime.now()}'); | ||
_startConnectionTimer(); | ||
})); | ||
|
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.
Would it be safer to set this callback before calling IOWebSocketChannel.connect
waqas-younas-deriv
approved these changes
Nov 18, 2024
behnam-deriv
approved these changes
Nov 19, 2024
ramin-deriv
commented
Nov 19, 2024
ramin-deriv
force-pushed
the
ramin/use_ready_flag
branch
from
November 20, 2024 07:48
dca26f1
to
77e6a08
Compare
behnam-deriv
approved these changes
Dec 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are two issues that this PR addresses:
Frequent Triggering of Disconnect Event
Cause: We use both WebSocket channel callbacks (e.g.,
onDone
,onError
) and the phone's network status (Wi-Fi, mobile data, VPNs, etc. usingconnectivity_plus
package) to determine connection status. Two of them can happen at the same time causing theConnectionCubit
to try to reconnect and emit a disconnect state causing the Reconnecting dialog to appear frequently.Solution: Use the WebSocket channel as the single source of truth for the connection status. By sending periodic pings to the server and checking ping success/failure timeout will confirm whether the connection is active or disconnected (the
websocket_channel
package that we use supports this out of the box). We can still keep the connectivity_plus and add it in the app itself to inform the user about network availability changes, but we don't interfere WebSocket's connection management with its status changes.Reduce the time takes to establish the WS connection
This is the time that it takes to establish WebSocket connection and dismiss the connecting dialog.
Cause: We currently confirm the WebSocket connection is established upon receiving the first pong response from the WebSocket server. A 5-second timer in
ConnectionCubit
sends periodic ping messages to the server, but if a ping was recently sent just before callingWebSocket.connect
, we might have to wait up to 5 seconds until the next ping, delaying confirmation of the connection.Solution: Keep the
ConnectionCubit
timer only for regular pings when the connection is stable, stopping it upon disconnection. After calling the connect method, we create and start another timer with a shorter interval duration inBinaryApi
to send ping requests, allowing us to confirm the connection status more quickly. We apply a exponential back-off strategy for this ping to avoid flooding the server if connecting to websocket fails. (We can even remove the timer inConnectionCubit
if we see it doesn't serve any other purpose)Note: The
WebSocketChannel
package provides aready
future to indicate when the WebSocket connection is established. Ideally, after callingWebSocket.connect
, we would wait for this future to complete before sending or receiving messages. However, this method is unreliable, as we sometimes fail to receive responses after the ready status. Until we find a more reliable way to utilize the ready future, we will continue with the above approach.