Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 11 commits into from
May 14, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
60cf4eb
Enforce new connection when last activity > than (idle interval + TTL)
funkyboy Apr 26, 2018
deb79a7
Improve connection freshness check
funkyboy Apr 27, 2018
014a1c8
Add test for connection freshness
funkyboy Apr 27, 2018
4dfd7da
Add test for connection when (ttl + idle interval) period has NOT pas…
funkyboy Apr 27, 2018
30e0e8f
Add test to verify channel reattach after a new connection has been e…
funkyboy May 2, 2018
3bb7493
Add refs to RTN15g spec items
funkyboy May 4, 2018
d1c7a3c
Reuse same channel for RTN15g3 test
funkyboy May 4, 2018
8565908
Close connections after tests
funkyboy May 4, 2018
c64f625
Move connection freshess check when connecting happens
funkyboy May 11, 2018
bd30f97
Remove calls to connect() from RTN15g tests
funkyboy May 11, 2018
4ea8dc0
Fix RTN17a test
funkyboy May 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Source/ARTRealtime.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,17 @@ - (ARTEventListener *)transitionSideEffects:(ARTConnectionStateChange *)stateCha

switch (stateChange.current) {
case ARTRealtimeConnecting: {

// RTN15g We want to enforce a new connection also when there hasn't been activity for longer than (idle interval + TTL)
if (stateChange.previous == ARTRealtimeDisconnected || stateChange.previous == ARTRealtimeSuspended) {
NSTimeInterval intervalSinceLast = [[NSDate date] timeIntervalSinceDate:_lastActivity];
if (intervalSinceLast > (_maxIdleInterval + _connectionStateTtl)) {
[self.connection setId:nil];
[self.connection setKey:nil];
[self.connection setSerial:0];
}
}

stateChangeEventListener = [self unlessStateChangesBefore:[ARTDefault realtimeRequestTimeout] do:^{
[weakSelf onConnectionTimeOut];
}];
Expand Down
114 changes: 113 additions & 1 deletion Spec/RealtimeClientConnection.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2649,6 +2649,118 @@ class RealtimeClientConnection: QuickSpec {
}
}
}

// RTN15g RTN15g1
context("when connection (ttl + idle interval) period has passed since last activity") {
let options = AblyTests.commonAppSetup()
// We want this to be > than the sum of customTtlInterval and customIdleInterval
options.disconnectedRetryTimeout = 5.0
var client: ARTRealtime!
var connectionId = ""
let customTtlInterval: TimeInterval = 1.0
let customIdleInterval: TimeInterval = 1.0

it("uses a new connection") {
client = AblyTests.newRealtime(options)
client.connect()
defer { client.close() }

waitUntil(timeout: testTimeout) { done in
client.connection.once(.connected) { _ in
expect(client.connection.id).toNot(beNil())
connectionId = client.connection.id!
client.connectionStateTtl = customTtlInterval
client.maxIdleInterval = customIdleInterval
client.connection.once(.disconnected) { _ in
let disconnectedAt = Date()
expect(client.connectionStateTtl).to(equal(customTtlInterval))
expect(client.maxIdleInterval).to(equal(customIdleInterval))
client.connection.once(.connecting) { _ in
let reconnectionInterval = Date().timeIntervalSince(disconnectedAt)
expect(reconnectionInterval).to(beGreaterThan(client.connectionStateTtl + client.maxIdleInterval))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

client.connection.once(.connected) { _ in
expect(client.connection.id).toNot(equal(connectionId))
done()
}
}
}
client.onDisconnected()
}
}
}
// RTN15g3
it("reattaches to the same channels after a new connection has been established") {
client = AblyTests.newRealtime(options)
client.connect()
defer { client.close() }
let channelName = "test-reattach-after-ttl"
let channel = client.channels.get(channelName)

waitUntil(timeout: testTimeout) { done in
client.connection.once(.connected) { _ in
connectionId = client.connection.id!
client.connectionStateTtl = customTtlInterval
client.maxIdleInterval = customIdleInterval
channel.attach { error in
if let error = error {
fail(error.message)
}
expect(channel.state).to(equal(ARTRealtimeChannelState.attached))
}
client.connection.once(.disconnected) { _ in
client.connection.once(.connecting) { _ in
client.connection.once(.connected) { _ in
expect(client.connection.id).toNot(equal(connectionId))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

channel.once(.attached) { _ in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

done()
}
}
}
}
client.onDisconnected()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

@mattheworiordan mattheworiordan May 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}
}

// RTN15g2
context("when connection (ttl + idle interval) period has NOT passed since last activity") {
let options = AblyTests.commonAppSetup()
options.disconnectedRetryTimeout = 5.0
var client: ARTRealtime!
var connectionId = ""
let customTtlInterval: TimeInterval = 20.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattheworiordan good spot. This is not needed.

let customIdleInterval: TimeInterval = 20.0

it("uses the same connection") {
client = AblyTests.newRealtime(options)
client.connect()
defer { client.close() }

waitUntil(timeout: testTimeout) { done in
client.connection.once(.connected) { _ in
expect(client.connection.id).toNot(beNil())
connectionId = client.connection.id!
client.connectionStateTtl = customTtlInterval
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for clarifying

client.maxIdleInterval = customIdleInterval
client.connection.once(.disconnected) { _ in
let disconnectedAt = Date()
expect(client.connectionStateTtl).to(equal(customTtlInterval))
expect(client.maxIdleInterval).to(equal(customIdleInterval))
client.connection.once(.connecting) { _ in
let reconnectionInterval = Date().timeIntervalSince(disconnectedAt)
expect(reconnectionInterval).to(beLessThan(client.connectionStateTtl + client.maxIdleInterval))
client.connection.once(.connected) { _ in
expect(client.connection.id).to(equal(connectionId))
done()
}
}
}
client.onDisconnected()
}
}
}
}

// RTN15h
context("DISCONNECTED message contains a token error") {
Expand Down Expand Up @@ -3313,7 +3425,7 @@ class RealtimeClientConnection: QuickSpec {
guard let error = stateChange?.reason else {
fail("Error is nil"); done(); return
}
expect(error.message).to(contain("bad response"))
expect(error.message).to(contain("Invalid key"))
done()
}
}
Expand Down