Prepare connection / region pinning#450
Conversation
| // Connect sequence successful | ||
| log("Connect sequence completed") | ||
| while true { | ||
| let region = try await resolveBestRegion() |
There was a problem hiding this comment.
I don't think we want to await region selection on initial connection?
The other SDKs don't block here and just continue immediately in order for region manager not to have any impact on connection speed
|
@hiroshihorie I'm digging into the older (stale?) PRs, anything left on this one? |
|
|
…it/client-sdk-swift into hiroshi/prepare-connection
|
|
||
| func shouldRequestSettings() -> Bool { | ||
| guard providedUrl.isCloud else { return false } | ||
| guard let lastRequested = state.lastRequested else { return true } |
There was a problem hiding this comment.
Just to double check, is it intentional to return true when lastRequested does not exist ?
There was a problem hiding this comment.
Yes, intentional. If lastRequested is nil, no settings have been fetched yet, so we should request them. This matches JS/Android behavior.
| } | ||
|
|
||
| func cancel() { | ||
| settingsFetchTask?.cancel() |
There was a problem hiding this comment.
should you also update the lastRequested if settingsFetchTask is being cancelled ?
There was a problem hiding this comment.
Intentionally not updating. Cancellation means we didn't get valid data: didn't complete the fetch. So we shouldn't block retry for 30 seconds. The next connect() should be allowed to fetch fresh region settings. 🤔
| Task { [weak self] in | ||
| _ = try? await task.value | ||
| // If the task failed before it could clear itself. | ||
| await self?.clearSettingsFetchTask(if: taskId) |
There was a problem hiding this comment.
maybe I miss something, I think we call clearSettingsFetchTask(if: taskId) more than once here
|
|
||
| await prepareAfterFailure() | ||
|
|
||
| let region = try await regionManager.resolveBest(token: token) |
There was a problem hiding this comment.
I wonder if we should do
do {
let region = try await regionManager.resolveBest(token: token)
nextUrl = region.url
nextRegion = region
} catch {
// handle the error in some better way ?
throw error
}
There was a problem hiding this comment.
I think it's probably fine as-is 🤔 Resolving regions shouldn't really fail if there's connectivity, and if it does, we probably shouldn't continue attempting to connect anyway. I believe the JS SDK has similar logic. If you have a specific suggestion, let me know 🙂
…it/client-sdk-swift into hiroshi/prepare-connection
xianshijing-lk
left a comment
xianshijing-lk
left a comment
There was a problem hiding this comment.
Lets land it asap, and make follow-up PRs if needed.
5 participants
Uh oh!
There was an error while loading. Please reload this page.