Chat web socket client #34
Conversation
| public protocol BackgroundTaskScheduler: Sendable { | ||
| /// It's your responsibility to finish previously running task. | ||
| /// | ||
| /// Returns: `false` if system forbid background task, `true` otherwise |
There was a problem hiding this comment.
Moved scheduler implementation from chat to core since there were additional fixes and the main actor annotations made it too hard to fix tests since using Task {} creates a delay.
Sources/StreamCore/WebSocket/Client/ConnectionRecoveryHandler.swift
Outdated
Show resolved
Hide resolved
| case let .disconnected(source): | ||
| let isWaitingForReconnect = webSocketConnectionState.isAutomaticReconnectionEnabled || source.serverError? | ||
| .isInvalidTokenError == true | ||
| let isWaitingForReconnect = webSocketConnectionState.isAutomaticReconnectionEnabled |
There was a problem hiding this comment.
Also from chat's implementation. isAutomaticReconnectionEnabled already has logic for invalid token
There was a problem hiding this comment.
hmm, we should check this on video.
There was a problem hiding this comment.
OK, some things are off here.
isAutomaticReconnectionEnabled returns false for isInvalidTokenError, but for some reason it was reversed here.
public var isAutomaticReconnectionEnabled: Bool {
guard case let .disconnected(source) = self else { return false }
switch source {
case let .serverInitiated(clientError):
if let wsEngineError = clientError?.underlyingError as? WebSocketEngineError,
wsEngineError.code == WebSocketEngineError.stopErrorCode {
// Don't reconnect on `stop` errors
return false
}
if let serverInitiatedError = clientError?.errorPayload {
if serverInitiatedError.isInvalidTokenError {
// Don't reconnect on invalid token errors
return false
}
if serverInitiatedError.isClientError && !serverInitiatedError.isTokenExpiredError {
// Don't reconnect on client side errors unless it is an expired token
// Expired tokens return 401, so it is considered client error.
return false
}
}
return true
case .systemInitiated:
return true
case .noPongReceived:
return true
case .userInitiated:
return false
case .timeout:
return false
}
}
There was a problem hiding this comment.
Looks like reversed to handle expired tokens. Looks messy.
There was a problem hiding this comment.
Fixed the issue with using errorPayload within that property, now it works correctly for everyone and there is no need for that OR statement.
There was a problem hiding this comment.
FYI: Tried these changes with Feeds and it seems to work just fine.
| case userInitiated | ||
|
|
||
| /// The connection timed out while trying to connect. | ||
| case timeout(from: WebSocketConnectionState) |
There was a problem hiding this comment.
Requirement from chat
| if serverInitiatedError.isClientError && !serverInitiatedError.isTokenExpiredError { | ||
| // Don't reconnect on client side errors unless it is an expired token | ||
| // Expired tokens return 401, so it is considered client error. |
|
|
||
| /// The current state the web socket connection. | ||
| @Atomic public private(set) var connectionState: WebSocketConnectionState = .initialized { | ||
| @Atomic public internal(set) var connectionState: WebSocketConnectionState = .initialized { |
| public weak var connectionStateDelegate: ConnectionStateDelegate? | ||
|
|
||
| public var connectURL: URL | ||
| public var connectRequest: URLRequest? |
There was a problem hiding this comment.
Chat needs URLRequest, internally URL is turned into URLRequest anyway, so makes sense to do it from the start
| } catch is ClientError.IgnoredEventType { | ||
| log.info("Skipping unsupported event type with payload: \(data.debugPrettyPrintedJSON)", subsystems: .webSocket) | ||
| return |
Generated by 🚫 Danger |
|
🔗 Issue Links
Fixes IOS-1205
🎯 Goal
Supporting changes for making web-socket client to fit into chat and make all of its existing testing flows to work
📝 Summary
Provide bullet points with the most important changes in the codebase.
🛠 Implementation
Provide a detailed description of the implementation and explain your decisions if you find them relevant.
🎨 Showcase
Add relevant screenshots and/or videos/gifs to easily see what this PR changes, if applicable.
🧪 Manual Testing Notes
Explain how this change can be tested manually, if applicable.
☑️ Contributor Checklist
docs-contentrepo