-
Notifications
You must be signed in to change notification settings - Fork 210
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 reconnection timeout handler not working in the token provider phase #3513
Changes from 6 commits
5098e45
62c1f3d
d86ae18
6a5d3bf
dce022f
cadfdf7
1b47595
f43af9b
bac6e84
0ed332f
4081aec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,10 @@ class ConnectionRepository { | |
self.timerType = timerType | ||
} | ||
|
||
func initialize() { | ||
webSocketClient?.initialize() | ||
} | ||
Comment on lines
+45
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be introduced. Otherwise, we won't report the status of the new connection to the delegates. This is especially important when using ChatClient as a singleton. This is the problem:
Since we don't reset the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when you call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added more details to point This approach actually makes sense, before the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall overview: stateDiagram-v2
[*] --> initialize
initialize --> ChatClient.Connect
ChatClient.Connect --> TokenProvider
TokenProvider --> ReconnectionTimeout: Fails
TokenProvider --> WebSocket.Connect: Success
ReconnectionTimeout --> disconnected
disconnected --> ChatClient.Connect
note right of WebSocket.Connect
State reported: .connecting
end note
note right of initialize
State reported: .initialize
end note
note right of disconnected
State reported: .disconnected
end note
note left of disconnected
On the second cycle, the state is not reported
Because it did not change from .disconnected
end note
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally it would be nice to see Especially because documentation says (one typo there):
That is confusing for the SDK user. My view is that I feel like it is OK in the PR to go for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, exactly, I had the same thinking as Toomas. Basically, this part: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, changing this would be very risky at the moment, that is why I decided not to do, and I remember I had a couple of issues trying it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @laevandus I changed that comment since it is internal either way. I tried changing the state to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree that we should try to tackle in another issue & PR. |
||
|
||
/// Connects the chat client the controller represents to the chat servers. | ||
/// | ||
/// When the connection is established, `ChatClient` starts receiving chat updates, and `currentUser` variable is available. | ||
|
@@ -95,14 +99,6 @@ class ConnectionRepository { | |
return | ||
} | ||
|
||
if connectionId == nil { | ||
if source == .userInitiated { | ||
log.warning("The client is already disconnected. Skipping the `disconnect` call.") | ||
} | ||
completion() | ||
return | ||
} | ||
|
||
Comment on lines
-98
to
-105
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an unnecessary optimization that just complicates things. For example, when we timeout, we don't have the connectionId, so the webSocketClient? .disconnect won't be called. Which means no connection status changes will be reported. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, this makes sense. |
||
// Disconnect the web socket | ||
webSocketClient?.disconnect(source: source) { [weak self] in | ||
// Reset `connectionId`. This would happen asynchronously by the callback from WebSocketClient anyway, but it's | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,6 +100,10 @@ class WebSocketClient { | |
self.eventNotificationCenter = eventNotificationCenter | ||
} | ||
|
||
func initialize() { | ||
connectionState = .initialized | ||
} | ||
|
||
/// Connects the web connect. | ||
/// | ||
/// Calling this method has no effect is the web socket is already connected, or is in the connecting phase. | ||
|
@@ -137,23 +141,18 @@ class WebSocketClient { | |
source: WebSocketConnectionState.DisconnectionSource = .userInitiated, | ||
completion: @escaping () -> Void | ||
) { | ||
connectionState = .disconnecting(source: source) | ||
engineQueue.async { [engine, eventsBatcher] in | ||
engine?.disconnect() | ||
|
||
eventsBatcher.processImmediately(completion: completion) | ||
switch connectionState { | ||
case .initialized, .disconnected, .disconnecting: | ||
connectionState = .disconnected(source: source) | ||
case .connecting, .waitingForConnectionId, .connected: | ||
connectionState = .disconnecting(source: source) | ||
Comment on lines
+144
to
+148
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when disconnecting, if we are already disconnected, we should not put the state to disconnecting since the engine won't do anything, and so it won't report the disconnected afterwards. For this reason, we need to instantly report as disconnected for this scenario. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems tricky, previously it seems we were also processing events. Can you explain a bit more why we delete that part? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not change the processing events part. The github diffing is not very clear, but the only thing changed here is the report of
Where does this happen? We report disconnected if we were not connected previously, so theres nothing to disconnect, that is why we instantly report that we are disconnected. If we are connecting, or waiting for connection or connected, then we need to call disconnect to the engine, so the engine will eventually report the disconnected state. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is quite clear actually: When the state is When the state is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, now after seeing the diagram and the explanations, it makes more sense, thanks. |
||
} | ||
} | ||
|
||
func timeout() { | ||
let previousState = connectionState | ||
connectionState = .disconnected(source: .timeout(from: previousState)) | ||
|
||
engineQueue.async { [engine, eventsBatcher] in | ||
engine?.disconnect() | ||
|
||
eventsBatcher.processImmediately {} | ||
eventsBatcher.processImmediately(completion: completion) | ||
} | ||
log.error("Connection timed out. `\(connectionState)", subsystems: .webSocket) | ||
} | ||
} | ||
|
||
|
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.
Should
reconnectionTimeoutHandler?.stop()
be called here as well or it does not matter?Thinking about the case of calling disconnect quickly after calling connect.
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.
It does not really matter, because this is triggered by the
reconnectionTimeoutHandler
which hasrepeats: false
, so it will be already stopped by the time it reaches here