-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add RTN15a #789
Add RTN15a #789
Conversation
This is needed for some tests, to suspend the behavior defined by RTN15a (reconnect immediately after disconnection).
@@ -68,6 +68,9 @@ NS_ASSUME_NONNULL_BEGIN | |||
|
|||
@property (readonly, getter=getClientOptions) ARTClientOptions *options; | |||
|
|||
/// Suspend the behavior defined in RTN15a, that is trying to immediately reconnect after a disconnection | |||
@property (readwrite, assign, nonatomic) BOOL suspendImmediateReconnection; |
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.
Why this flag? If the client isn't respecting the spec then it should not be a flag to be activated only for the test suite, right? Maybe I'm missing something.
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.
The spec says If a Connection transport is disconnected unexpectedly or if a token expires, then the Connection manager will immediately attempt to reconnect and restore the connection state.
so the client should immediately attempt to reconnect always and it doesn't. It's a bug then, where iOS does not retry a connection when it fails immediately. It waits 15s now right?
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.
@ricardopereira correct, now it waits 15s.
The flag is needed because sometimes you need a "disconnect and stay in the disconnected for x seconds" case. For example when you are testing that there's a new connection id if you connect after (ttl + idle interval). Without this flag you'd have an immediate connection retry.
We had to do something similar in Java.
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.
Ok, understood.
Am I missing it or is there a test that verifies there is an immediate reconnection? |
@paddybyers been so focused on avoiding immediate reconnection in tests that I forgot to test if it actually happens :) |
LGTM, although I worry that anything with only a 2s margin of error will break in Travis. I think t's safe to check the reconnect time is anything in the interval 0 .. 10s, because at least then you know it's not waiting the usual 15s. |
Travis tests were passing but let's make sure we have more leeway e16c7a6 |
e16c7a6
to
ca19426
Compare
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.
LGTM, thanks
(RTN15a) If a Connection transport is disconnected unexpectedly or if a token expires, then the Connection manager will immediately attempt to reconnect and restore the connection state. Connection state recovery is provided by the Ably service and ensures that whilst the client is disconnected, all events are queued and channel state is retained on the Ably servers. When a new connection is made with the correct connection recovery key, the client is able to catch up by receiving the queued ProtocolMessages from Ably.
disconnectedRetryTimeout
suspendImmediateReconnection
property to prevent an immediate reconnection after a disconnection happens. Handy for tests.