Enforce new connection when last activity > than (idle interval + TTL) #719

funkyboy · 2018-04-26T15:11:35Z

Enforces a new connection when the last activity is > than (_maxIdleInterval + _connectionStateTtl)

Mimics this behavior in JS, called when a connection gets started.

ricardopereira · 2018-04-26T22:34:58Z

Source/ARTRealtime.m

+    NSTimeInterval intervalSinceLast = [[NSDate date] timeIntervalSinceDate:_lastActivity];
+
+    // We want to enforce a new connection also when there hasn't been activity for longer than (idle interval + TTL)
+    if(self.connection.state_nosync == ARTRealtimeClosing || intervalSinceLast > (_maxIdleInterval + _connectionStateTtl)) {


Hmm, I think that could easily create too many connections. I'll check that in more detail tomorrow.

Confirmed, tests are failing and from the tests...

[15:26:38]: ▸ ✗ Connection__connection_request_fails__should_transition_to_Failed_state_because_the_token_is_invalid_and_not_renewable, expected to equal <15>, got <50> [15:26:38]: ▸ ✗ Connection__connection_request_fails__should_transition_to_Failed_state_because_the_token_is_invalid_and_not_renewable, Should have only one connection request fail

this is not good.

ricardopereira · 2018-04-26T22:36:24Z

Source/ARTRealtime.m

+    NSTimeInterval intervalSinceLast = [[NSDate date] timeIntervalSinceDate:_lastActivity];
+
+    // We want to enforce a new connection also when there hasn't been activity for longer than (idle interval + TTL)
+    if(self.connection.state_nosync == ARTRealtimeClosing || intervalSinceLast > (_maxIdleInterval + _connectionStateTtl)) {


Confirmed, tests are failing and from the tests...

[15:26:38]: ▸ ✗ Connection__connection_request_fails__should_transition_to_Failed_state_because_the_token_is_invalid_and_not_renewable, expected to equal <15>, got <50> [15:26:38]: ▸ ✗ Connection__connection_request_fails__should_transition_to_Failed_state_because_the_token_is_invalid_and_not_renewable, Should have only one connection request fail

this is not good.

funkyboy · 2018-04-27T07:37:01Z

Took another stab deb79a7

paddybyers · 2018-04-27T07:40:17Z

Have a look at the tests in ably/ably-ruby#146 for the cases we need to test

funkyboy · 2018-04-27T07:52:09Z

@paddybyers Will do

paddybyers · 2018-04-27T08:33:41Z

ricardopereira approved these changes 32 minutes ago

I do think we should have test cases included before approving the PR.

mattheworiordan · 2018-04-27T09:33:21Z

@funkyboy I think we should get into the habit with spec changes of writing tests that fail, then making them pass. This approach IMHO is in most cases wrong. If you look back at how we worked with @ricardopereira in the past, he wrote tests that failed but met the spec, then Toni wrote the code. I am not sure anyone can review this code in any meaningful way without tests.

funkyboy · 2018-04-27T09:37:31Z

@mattheworiordan will do. Won't merge this without having tests, which I am working on.

funkyboy · 2018-04-27T12:49:53Z

@mattheworiordan you can read

delay(reconnectAfterInterval) {
  client.connect()
}

as the counterpart of the following Ruby

sleep(reconnectAfterInterval)
client.connect

mattheworiordan · 2018-04-27T12:52:54Z

you can read
delay(reconnectAfterInterval) {
client.connect()
}
as the counterpart of the following Ruby
sleep(reconnectAfterInterval)
client.connect

Ok, so then if the client is running in it's own thread, whilst the test is sleeping for X time, what's stopping it from reconnecting itself automatically?

funkyboy · 2018-04-27T13:03:24Z

It puts to sleep the whole test context, on the main thread: https://github.com/ably/ably-ios/blob/develop/Spec/TestUtilities.swift#L498. The client also belongs to that thread.

funkyboy · 2018-04-27T16:01:33Z

@ricardopereira added test. Ready for another look.
cc @paddybyers

mattheworiordan · 2018-04-27T16:44:23Z

Ready for another look.

@funkyboy I don't think that is sufficient. See https://github.com/ably/ably-ruby/pull/146/files#diff-6994e351b062efe17b0b5c88ac75991f for comparison.

I test both positive & negative conditions, as well as the channel attach requirements in the specification. I don't believe this is comprehensive enough.

funkyboy · 2018-04-27T17:04:24Z

Oh you have added more Ruby tests since yesterday :)

Add test when the reconnection happens before (idle interval + TTL) has passed
Add test to check it still reattaches to channels after a new connection has been established. See Ruby example

…sed since last activity

…stablished

funkyboy · 2018-05-02T17:06:34Z

@mattheworiordan @paddybyers @ricardopereira ready for another look

mattheworiordan

Few small comments

mattheworiordan · 2018-05-02T17:13:28Z

Spec/RealtimeClientConnection.swift

+                            client.connection.once(.connected) { _ in
+                                expect(client.connection.id).toNot(beNil())
+                                connectionId = client.connection.id!
+                                client.connectionStateTtl = customTtlInterval


Does the client really allow these intervals to be changed once it is instanced? Or is this a test thing that allows us to "hack" these values on a running client? Obviously if customers tried the same thing they'd get very odd and unpredictable behaviour.

For our customers connectionStateTtl is a private readonly property.

We have a bunch of methods to expose and set private variables for whitebox testing.
connectionStateTtl is one of those https://github.com/ably/ably-ios/blob/develop/Source/ARTRealtime%2BPrivate.h#L51

Ok, thanks for clarifying

mattheworiordan · 2018-05-02T17:15:13Z

Spec/RealtimeClientConnection.swift

@@ -2690,6 +2690,42 @@ class RealtimeClientConnection: QuickSpec {
                            }
                        }
                    }
+
+                    it("reattaches to the same channels after a new connection has been established") {


Should we not includ the specification IDs in all tests so they are greppable?

See https://github.com/ably/ably-ruby/pull/146/files#diff-6994e351b062efe17b0b5c88ac75991fR558 for example.

@mattheworiordan done 3bb7493

mattheworiordan · 2018-05-02T17:18:55Z

Spec/RealtimeClientConnection.swift

+                                    client.connection.once(.connecting) { _ in
+                                        client.connection.once(.connected) { _ in
+                                            expect(client.connection.id).toNot(equal(connectionId))
+                                            let newChannel = client.channels.get(channelName)


Why are you instancing a new channel? The title of this tests says "reattaches to the same channels after a new connection has been established", yet you've just called get to get a new channel. Should you not be checking that the existing channel object you have emits an additional attached event once the connection becomes connected, indicating that it's been reattached by the client? See https://github.com/ably/ably-ruby/pull/146/files#diff-6994e351b062efe17b0b5c88ac75991fR714

@mattheworiordan done d1c7a3c

ricardopereira · 2018-05-04T12:55:53Z

Spec/RealtimeClientConnection.swift

+                        // We want this to be > than the sum of the two above
+                        let reconnectAfterInterval: TimeInterval = 5.0
+                        client = AblyTests.newRealtime(options)
+                        client.connect()


@funkyboy I usually add a defer closing the current connection. The connection will remain active. Something like defer { client.close() }.

@ricardopereira done 8565908

ricardopereira

Looks good but I think you should close the current realtime connections.

funkyboy · 2018-05-09T17:07:14Z

@mattheworiordan If you don't have any further observations, I'd merge this.

mattheworiordan · 2018-05-09T19:01:47Z

@funkyboy I was not issued with a request to review this again so I was not aware it was awaiting a review. This looks good to me.

paddybyers

LGTM, thanks

funkyboy · 2018-05-10T14:38:53Z

After talking to @paddybyers and after working on the Java version of this feature I realized that these tests are wrong, in that they should wait for the suspended state (after disconnecting) and then perform checks. Here's a new todo list:

Change tests to follow this sequence:
1. connect
2. extract data / mock values
3. disconnect
4. wait for the connection to go into suspended mode
5. reconnect and wait for the connection to reconnect
6. verify assertions
Change implementation accordingly

funkyboy · 2018-05-11T15:54:01Z

After another discussion about RTN15g it turns out that this (fresh connection after connectionTtl + idle timer has passed) doesn't need to go through the suspended state.

I tweaked the tests so that the reconnection happens via the state machine instead of calling connect().
That surfaced a bug (that I fixed) because the freshness check was performed only when connect() was called directly.

cc @paddybyers @mattheworiordan @ricardopereira

mattheworiordan · 2018-05-11T22:41:13Z

Ok, logic seems good

paddybyers

LGTM, thanks

mattheworiordan

I know this is merged, but I believe there are issues here

mattheworiordan · 2018-05-15T08:22:17Z

Spec/RealtimeClientConnection.swift

+                                    expect(client.maxIdleInterval).to(equal(customIdleInterval))
+                                    client.connection.once(.connecting) { _ in
+                                        let reconnectionInterval = Date().timeIntervalSince(disconnectedAt)
+                                        expect(reconnectionInterval).to(beGreaterThan(client.connectionStateTtl + client.maxIdleInterval))


Why is this the case? I don't see any explicit request to pause / delay the reconnecting. What has caused this delay then?

@mattheworiordan It was this option, which tries the reconnection after 5s the client has been in a DISCONNECTED state https://github.com/ably/ably-ios/blob/develop/Spec/RealtimeClientConnection.swift#L2657

But this is not according to the spec. The client will retry to reconnect immediately after disconnect, see https://docs.ably.io/client-lib-development-guide/features/#RTN15a, not wait disconnectedRetryTimeout. It only waits disconnectedRetryTimeout on subsequent requests.

mattheworiordan · 2018-05-15T08:22:59Z

Spec/RealtimeClientConnection.swift

+                                        }
+                                    }
+                                }
+                                client.onDisconnected()


Why is this done before the attach is completed? The sequence of events for this test is wrong i.e. the initial attach may only happen once the connection becomes connected again, in fact that is very likely.

@mattheworiordan I don't understand. Would you prefer to see this within the attach block and right after this assertion? https://github.com/ably/ably-ios/blob/develop/Spec/RealtimeClientConnection.swift#L2708

Yes, exactly, otherwise how do we know that the channel attached first and thus gets reattached after reconnected?

mattheworiordan · 2018-05-15T08:24:06Z

Spec/RealtimeClientConnection.swift

+                                client.connection.once(.disconnected) { _ in
+                                    client.connection.once(.connecting) { _ in
+                                        client.connection.once(.connected) { _ in
+                                            expect(client.connection.id).toNot(equal(connectionId))


Why is the connection ID changing? There is no pause as far as I can see.

This it block uses the same settings declared up here https://github.com/ably/ably-ios/blob/develop/Spec/RealtimeClientConnection.swift#L2655

As above, there should be no pause according to https://docs.ably.io/client-lib-development-guide/features/#RTN15a

mattheworiordan · 2018-05-15T08:24:29Z

Spec/RealtimeClientConnection.swift

+                                    client.connection.once(.connecting) { _ in
+                                        client.connection.once(.connected) { _ in
+                                            expect(client.connection.id).toNot(equal(connectionId))
+                                            channel.once(.attached) { _ in


You are not checking that the attached ChannelStateChange has a flag indicating loss of continuity on the channel.

@mattheworiordan you mean I should check if resumed is true?

Yes exactly https://docs.ably.io/client-lib-development-guide/features/#TH4

mattheworiordan · 2018-05-15T08:24:55Z

Spec/RealtimeClientConnection.swift

+                    options.disconnectedRetryTimeout = 5.0
+                    var client: ARTRealtime!
+                    var connectionId = ""
+                    let customTtlInterval: TimeInterval = 20.0


Why even set this, surely a standard connection should reconnect automatically without any custom settings?

@mattheworiordan good spot. This is not needed.

funkyboy · 2018-05-15T16:28:30Z

@mattheworiordan Made an issue with your feedback on this PR after merge.

Enforce new connection when last activity > than (idle interval + TTL)

60cf4eb

funkyboy requested a review from ricardopereira April 26, 2018 15:11

ricardopereira reviewed Apr 26, 2018

View reviewed changes

ricardopereira suggested changes Apr 26, 2018

View reviewed changes

Improve connection freshness check

deb79a7

ricardopereira approved these changes Apr 27, 2018

View reviewed changes

Add test for connection freshness

014a1c8

funkyboy added 2 commits April 27, 2018 21:05

Add test for connection when (ttl + idle interval) period has NOT pas…

4dfd7da

…sed since last activity

Add test to verify channel reattach after a new connection has been e…

30e0e8f

…stablished

mattheworiordan requested changes May 2, 2018

View reviewed changes

funkyboy added 2 commits May 4, 2018 11:40

Add refs to RTN15g spec items

3bb7493

Reuse same channel for RTN15g3 test

d1c7a3c

ricardopereira reviewed May 4, 2018

View reviewed changes

ricardopereira suggested changes May 4, 2018

View reviewed changes

Close connections after tests

8565908

ricardopereira approved these changes May 4, 2018

View reviewed changes

paddybyers approved these changes May 9, 2018

View reviewed changes

funkyboy added 2 commits May 11, 2018 17:19

Move connection freshess check when connecting happens

c64f625

Remove calls to connect() from RTN15g tests

bd30f97

Fix RTN17a test

4ea8dc0

funkyboy force-pushed the add-connection-freshness-check branch from 3917a81 to 4ea8dc0 Compare May 11, 2018 16:56

paddybyers approved these changes May 14, 2018

View reviewed changes

funkyboy merged commit 67d1d94 into develop May 14, 2018

funkyboy deleted the add-connection-freshness-check branch May 14, 2018 16:11

funkyboy mentioned this pull request May 14, 2018

Implement connection state freshness check #645

Closed

mattheworiordan reviewed May 15, 2018

View reviewed changes

funkyboy mentioned this pull request May 15, 2018

Improve RTN15G tests #725

Closed

4 tasks

mattheworiordan mentioned this pull request May 16, 2018

Improve RTN15G tests #726

Merged

Enforce new connection when last activity > than (idle interval + TTL) #719

Enforce new connection when last activity > than (idle interval + TTL) #719

Conversation

funkyboy commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyboy commented Apr 27, 2018

paddybyers commented Apr 27, 2018

funkyboy commented Apr 27, 2018

paddybyers commented Apr 27, 2018

mattheworiordan commented Apr 27, 2018

funkyboy commented Apr 27, 2018

funkyboy commented Apr 27, 2018

mattheworiordan commented Apr 27, 2018

funkyboy commented Apr 27, 2018

funkyboy commented Apr 27, 2018

mattheworiordan commented Apr 27, 2018

funkyboy commented Apr 27, 2018 • edited Loading

funkyboy commented May 2, 2018

mattheworiordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardopereira left a comment

Choose a reason for hiding this comment

funkyboy commented May 9, 2018

mattheworiordan commented May 9, 2018

paddybyers left a comment

Choose a reason for hiding this comment

funkyboy commented May 10, 2018 • edited Loading

funkyboy commented May 11, 2018 • edited Loading

mattheworiordan commented May 11, 2018

paddybyers left a comment

Choose a reason for hiding this comment

mattheworiordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyboy commented May 15, 2018

funkyboy commented Apr 27, 2018 •

edited

Loading

funkyboy commented May 10, 2018 •

edited

Loading

funkyboy commented May 11, 2018 •

edited

Loading

mattheworiordan May 15, 2018 •

edited

Loading