[SDK-3926] Treat all transport errors as recoverable #1823
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
github-actions
bot
temporarily deployed
to
staging/pull/1823/features
github-actions
bot
temporarily deployed
to
staging/pull/1823/jazzydoc
lawrence-forooghian
force-pushed
the
1817-treat-all-transport-errors-as-recoverable
branch
from
82d6180 to
e716768
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1823/features
|
@paddybyers this is pretty much the PR — I've got to pop out shortly and will add a further test when back to check that an arbitrary error does not provoke a transition to |
github-actions
bot
temporarily deployed
to
staging/pull/1823/jazzydoc
|
Thanks. See ably/specification#171 (comment). So if these changes implement that approach, we're good. |
lawrence-forooghian
force-pushed
the
1817-treat-all-transport-errors-as-recoverable
branch
from
e716768 to
46ee0b0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1823/features
github-actions
bot
temporarily deployed
to
staging/pull/1823/jazzydoc
|
As things stand in this PR:
|
|
@paddybyers is there more that needs to be done than what I've detailed above? |
maratal
approved these changes
Nov 6, 2023
I’m going to need to change this test in an upcoming commit and want the original intent to be clear before I do.
The use of the non-completing authCallback already ensures a timeout.
Want to add a new test with another failure reason.
RTN14d says that a transport error should be considered recoverable if it is > a network failure, a timeout such as RTN14c, or a disconnected > response, other than a token failure RTN14b) However, it does not define what it means by a "network failure", leading to each platform having to provide its own interpretation. In particular, a client recently told us that they’ve been seeing lots of OSStatus 9806 (errSSLClosedAbort) errors. This appears to indicate some sort of failure to perform an SSL handshake. We don’t understand the cause of this issue but we noticed that it was causing the client to transition to the FAILED state. Speaking to Paddy, he said that this error should be provoking a connection retry, not failure. And more broadly, he indicated that, basically, _all_ transport errors should be considered recoverable. So, that’s what we do here. He’s raised a specification issue [1] for us to properly specify this and to decide if there are any nuances to consider, but is keen for us to implement this broad behaviour in ably-cocoa ASAP to help the customer experiencing the errSSLClosedAbort errors. Resolves #1817. [1] ably/specification#171
lawrence-forooghian
force-pushed
the
1817-treat-all-transport-errors-as-recoverable
branch
from
46ee0b0 to
86b7cc7
Compare
paddybyers
approved these changes
Nov 7, 2023
lawrence-forooghian
deleted the
1817-treat-all-transport-errors-as-recoverable
branch
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.
This treats all transport errors as recoverable from the point of view of RTN14d.
The immediate reason for doing this is so that an error of OSStatus 9806 (
errSSLClosedAbort) doesn't cause the client to transition to theFAILEDstate. But, as described in the commit message, this also reflects the broader approach that @paddybyers wants us to take for handling transport errors.Resolves #1817.