Fix internet-up.ably-realtime.com checks #961
Merged
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
The spec says: > In the case of an error necessitating use of an alternative host > (see RTN17d), the Connection manager should first check if an internet > connection is available [...] Changing fallback hosts is an instance of such an error, not just going from non-fallback to fallback. It also makes more sense.
Spec/RealtimeClientConnection.swift
Outdated
| @@ -3560,16 +3560,19 @@ class RealtimeClientConnection: QuickSpec { | |||
| TestProxyTransport.network = .hostUnreachable | |||
| defer { TestProxyTransport.network = nil } | |||
|
|
|||
| var urlConnections = [NSURL]() | |||
| var urls = [NSURL]() | |||
There was a problem hiding this comment.
We can just use URL instead of NSURL and avoid those casts.
Spec/RealtimeClientConnection.swift
Outdated
| if Optional(fallbackHost) == resultFallbackHosts.last { | ||
| continue | ||
| } | ||
| // Host changed; should've had a internet check before. |
There was a problem hiding this comment.
Small typo: "should've had a internet"
| } | ||
| // wss://[a-e].ably-realtime.com: when a 401 occurs because of the `xxxx:xxxx` key | ||
| client.connection.once(.failed) { error in | ||
| done() |
There was a problem hiding this comment.
Does this need a partialDone?
There was a problem hiding this comment.
The connection is going to get either to DISCONNECTED or to FAILED. Anyway, it was this way before.
ricardopereira
previously approved these changes
Dec 27, 2019
|
CI is failing (sometimes): |
|
@tcard Does the CI pass in your machine? |
ricardopereira
previously approved these changes
Jan 17, 2020
It does, but this really is failing too much on CI. I'll come back to this at some point. |
|
CI failures seem unrelated to this fix and just seem related to the general theme of Travis just hanging for too long. |
|
@ricardopereira PTAL, GitHub dismissed your review after a merge commit. |
ricardopereira
approved these changes
Jan 26, 2020
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.
Run them on each fallback host change, and don't ignore the result.
Fixes #952.
Commit acdb52c supersedes #959 and fixes #931.