Add connection freshness check #390

funkyboy · 2018-05-09T16:49:05Z

Fix #358

Add connection freshness check
Test reconnection after ttl + idleTime has passed
Test reconnection before ttl + idleTime has passed
Test channels attached during the first connection are correctly reattached after reconnecting when ttl + idleTime has passed
Include heartbeats in lastActivityTime
Fix failing channels_are_reattached_after_reconnecting_when_statettl_plus_idleinterval_has_passed. Reason: channels stay in an ATTACHED state even when the connection freshness check disposes of the old connection and creates a new one.

Should wait for the connection to be suspended

and extract it into a method

funkyboy · 2018-05-10T17:45:49Z

Rebased on add-connectionStateTtl

paddybyers

LGTM, thanks

paddybyers · 2018-05-10T20:45:19Z

lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java

+				@Override
+				public void onConnectionStateChanged(ConnectionStateChange state) {
+					try {
+						Field connectionStateField = ably.connection.connectionManager.getClass().getDeclaredField("connectionStateTtl");


We could consider having a helper for this? Maybe it doesn't really simplify

paddybyers · 2018-05-14T23:15:12Z

lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java

+							 * We want it to stay into a disconnected state long enough
+							 * so that the connection becomes stale.
+							 */
+							ably.connection.connectionManager.wait(waitInDisconnectedState);


I don't understand what the aim of this wait() is for. Won't this code be blocking the connectionmanager thread? Was that the intention?

I believe that was the intention i.e. the time required for connection to become stale will pass, even if blocked.

@paddybyers What @mattheworiordan is correct. Every attempt to go into a disconnected state using .requestState(ConnectionState.disconnected) will try to reconnect immediately. We want the connection manager to stay put in the disconnected state for a bit.

Every attempt to go into a disconnected state using .requestState(ConnectionState.disconnected) will try to reconnect immediately.

This is no longer true because I added the private method to suppress retries. I don't like blocking the connectionmanager thread explicitly here, because it stops any other processing from happening by the connectionmanager, and so we can no longer validly draw any conclusions about what its behaviour will be in a real situation. I think this wait() should be removed, and instead this test updated to use the new method.

mattheworiordan

I'll review this in more detail later, but I see some issues here I am afraid

mattheworiordan · 2018-05-15T05:57:41Z

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java

@@ -205,6 +205,7 @@ public String getHost() {
 	 *********************/

 	public void connect() {
+


Why white space here?

mattheworiordan · 2018-05-15T05:59:19Z

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java

@@ -451,6 +452,7 @@ public void onMessage(ITransport transport, ProtocolMessage message) throws Ably
 		if (Log.level <= Log.VERBOSE)
 			Log.v(TAG, "onMessage(): " + message.action + ": " + new String(ProtocolSerializer.writeJSON(message)));
 		try {
+			lastActivity = System.currentTimeMillis();


What about WebSocket heartbeats? I believe this code will only catch protocol messages, yet WebSocket heartbeats are considered valid for freshness activity.

If we don't allow for WebSocket heartbeats we're now going to incorrectly move people into the suspended state now.

In fact there already is support for tracking activity at the transport level, which includes heartbeats:
https://github.com/ably/ably-java/blob/develop/lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java#L188

The staleness check needs to be based on this instead of introducing a new mechanism here.

mattheworiordan · 2018-05-15T05:59:35Z

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java

+	private boolean checkConnectionStale() {
+	    if(lastActivity == 0) {
+	        return false;
+        }


Indention issues.

mattheworiordan · 2018-05-15T06:01:46Z

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java

 	private void handleStateChange(StateIndication stateChange) {
 		/* if we have had a disconnected state indication
 		 * from the transport then we have to decide whether
 		 * to transition to closed, disconnected to suspended depending
 		 * on when we last had a successful connection */
 		if(stateChange.state == ConnectionState.disconnected) {
+			if(checkConnectionStale()) {
+				requestState(ConnectionState.suspended);


Should we not add a Log.v(TAG) statement here for debugging purposes?

It would be useful in future to know why a connection moved suddenly to suspended

The more I think about this change the less I like it. The spec changes for stale connection state are at https://github.com/ably/docs/pull/331/files and make no mention of this change, so is it really sensible now to change this? As far as I can tell from the spec at https://docs.ably.io/client-lib-development-guide/features/#RTN14e, we state a connection moves to the SUSPENDED state only following a connection failure attempt, yet now we are moving to the suspended state as a result of DISCONNECTED. Additionally, what side effects may there be now that are unexpected? For example, previously when a connection first became disconnected it would immediately retry to connect, so if a device went to sleep, as soon as it wakes up, it will reconnect following it moving to the DISCONNECTED state. Now, we can't be sure that that happens because moving to the SUSPENDED state will result in the connection only being tried every 30 seconds. So have we now with this change potentially made our clients 30s less responsive by default when a device comes out of sleep? I would really like to follow why we are explicitly making this change here in this PR when as far as I can tell, this change has nothing to do with the spec change.

See https://www.ably.io/documentation/realtime/connection#connection-state-explained btw. In the examples we provide in the sequence of events, we never show a state change from disconnected -> suspended either.

@funkyboy FYI @paddybyers explained to me that this on state change handler is for the transport, not the connection, so my comments above are incorrect. Sorry, I misunderstood

mattheworiordan · 2018-05-15T06:02:27Z

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java

+		if(intervalSinceLastActivity > (maxIdleInterval + connectionStateTtl)) {
+			/* RTN15g1, RTN15g2 Force a new connection if the previous one is stale */
+			if(connection.id != null) {
+				connection.id = null;


I think we should add a log message here about why the connection state is explicitly being cleared

@mattheworiordan done 0a1c622

mattheworiordan · 2018-05-15T06:04:02Z

lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java

+            /* Since newTtl + newIdleInterval has passed we expect the connection to go into a suspended state */
+            connectionWaiter.waitFor(ConnectionState.suspended);
+
+			ably.connect();


Indentation issues

mattheworiordan · 2018-05-15T06:04:48Z

lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java

+						@Override
+						public void onChannelStateChanged(ChannelStateChange stateChange) {
+							System.out.println("Channel reattached after a new connection has been established");
+							ably.close();


How we do know the test ends at this point? Surely we need a clause here to confirm that this test has finished i.e. could this test not complete before this, or am I not reading this correctly?

mattheworiordan

Please see my comments, I don't think this is ready

mattheworiordan · 2018-05-15T07:47:55Z

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java

@@ -669,12 +671,34 @@ private void handleStateRequest() {
 		requestedState = null;
 	}

+	private boolean checkConnectionStale() {
+	    if(lastActivity == 0) {
+	        return false;


Why is this not true, as if the last activity is not recorded, I assume the connection is stale / not initialised? In fact, what does the boolean result even represent. This method is called checkConnectionStale, yet returns a boolean value. So does that mean connection is stale if true, if so, should this not be isConnectionStale. And if it is that, it should not have side effects. This method IMHO is doing two things and it should not. Is it an accessor or a modifier?

mattheworiordan · 2018-05-15T08:05:16Z

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java

 	private void handleStateChange(StateIndication stateChange) {
 		/* if we have had a disconnected state indication
 		 * from the transport then we have to decide whether
 		 * to transition to closed, disconnected to suspended depending
 		 * on when we last had a successful connection */
 		if(stateChange.state == ConnectionState.disconnected) {
+			if(checkConnectionStale()) {
+				requestState(ConnectionState.suspended);


The more I think about this change the less I like it. The spec changes for stale connection state are at https://github.com/ably/docs/pull/331/files and make no mention of this change, so is it really sensible now to change this? As far as I can tell from the spec at https://docs.ably.io/client-lib-development-guide/features/#RTN14e, we state a connection moves to the SUSPENDED state only following a connection failure attempt, yet now we are moving to the suspended state as a result of DISCONNECTED. Additionally, what side effects may there be now that are unexpected? For example, previously when a connection first became disconnected it would immediately retry to connect, so if a device went to sleep, as soon as it wakes up, it will reconnect following it moving to the DISCONNECTED state. Now, we can't be sure that that happens because moving to the SUSPENDED state will result in the connection only being tried every 30 seconds. So have we now with this change potentially made our clients 30s less responsive by default when a device comes out of sleep? I would really like to follow why we are explicitly making this change here in this PR when as far as I can tell, this change has nothing to do with the spec change.

mattheworiordan · 2018-05-15T08:05:51Z

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java

 	private void handleStateChange(StateIndication stateChange) {
 		/* if we have had a disconnected state indication
 		 * from the transport then we have to decide whether
 		 * to transition to closed, disconnected to suspended depending
 		 * on when we last had a successful connection */
 		if(stateChange.state == ConnectionState.disconnected) {
+			if(checkConnectionStale()) {


Appears to be typos in the comments above.

@mattheworiordan @paddybyers This is quite a rare case. Might happen when the app is open, the connection is active but nothing is being sent for quite a while. I don't see it harmful but I agree with Matt that it's beyond the scope of this spec change.

Well I misunderstood that stateChange was a connection state change, if it's a transport state change I am in fact OK with this. Sorry for the noise

mattheworiordan · 2018-05-15T08:12:36Z

lib/src/main/java/io/ably/lib/transport/ConnectionManager.java

 	private void handleStateChange(StateIndication stateChange) {
 		/* if we have had a disconnected state indication
 		 * from the transport then we have to decide whether
 		 * to transition to closed, disconnected to suspended depending
 		 * on when we last had a successful connection */
 		if(stateChange.state == ConnectionState.disconnected) {
+			if(checkConnectionStale()) {
+				requestState(ConnectionState.suspended);


See https://www.ably.io/documentation/realtime/connection#connection-state-explained btw. In the examples we provide in the sequence of events, we never show a state change from disconnected -> suspended either.

mattheworiordan · 2018-05-15T08:14:20Z

lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java

+			ably.connection.connectionManager.requestState(ConnectionState.disconnected);
+			connectionWaiter.waitFor(ConnectionState.disconnected);
+
+            ably.connection.once(ConnectionEvent.connected, new ConnectionStateListener() {


Indentation issues

mattheworiordan · 2018-05-15T08:15:39Z

lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java

+				}
+			});
+
+			connectionWaiter.waitFor(ConnectionState.connected);


Shouldn't this be wait for closed so that you can be 100% confident the onConnectionStateChanged ran above? Otherwise, this is racy and the test could complete before those assertions.

@mattheworiordan given that we have waited for disconnected before I think this is always gonna wait for connected and thus trigger the handler. That said, waiting for closed can't harm :) 6892ecc

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

lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java

+			});
+			channel.attach();
+
+            /* Before disconnecting wait that newTtl + newIdleInterval has passed */


Indentation issues again

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

lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java

+					assertEquals("Channel is attached", stateChange.current, ChannelState.attached);
+				}
+			});
+			channel.attach();


You're not explicitly awaiting the channel attached, and you cannot be sure the onChannelStateChanged handler ran with this code path.

@mattheworiordan done 0a14d26

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

lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java

+			ably.connection.connectionManager.requestState(ConnectionState.disconnected);
+
+            /* Since newTtl + newIdleInterval has passed we expect the connection to go into a suspended state */
+            connectionWaiter.waitFor(ConnectionState.suspended);


I am not sure I agree with this, as above... I think the connection state should be lost, but there is no reason it should become suspended according to the spec.

mattheworiordan · 2018-05-15T08:19:49Z

lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java

+						}
+					});
+				}
+			});


You are not waiting for those to close, this entire section could easily never be run as part of this test.

@mattheworiordan done 77f3cf8

paddybyers · 2018-05-15T08:27:43Z

The more I think about this change the less I like it.

I think you're misunderstanding what this is doing. This is the check, that already existed, which determines which state to enter (DISCONNECTED vs SUSPENDED) when there is an indication of disconnection (which includes a failure to connect) from the transport. It's not a transition of the connection state from DISCONNECTED to SUSPENDED.

The only change here that is being made in this PR is that the staleness check is being used at the time of a new connection attempt, in addition to at the time of a disconnection or failed connection attempt.

…attached channels do not enter suspended when handling a discontinuity; add a private method on ConnectionManager to support tests by dropping a connection and not automatically retrying it

… ensure that the resulting (clean) connection is handled as a resume failure, triggering reattach for discontinuity of attached channels

funkyboy · 2018-05-17T09:37:51Z

@paddybyers @mattheworiordan ready for another look

funkyboy · 2018-05-17T15:03:02Z

Note that this will be merged into https://github.com/ably/ably-java/tree/add-connectionStateTtl, which is part of another ongoing PR #389

mattheworiordan

AFAICT this looks good. However, do you know why the build failed?

mattheworiordan · 2018-05-17T16:13:45Z

lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java

 			connectionWaiter.waitFor(ConnectionState.disconnected);
+			assertEquals("Disconnected state was not reached", ConnectionState.disconnected, ably.connection.state);
+			System.out.println("=============================== DISCONNECTED ");


What are these for lines for?

That were meant for debugging but have been deleted in this commit 6ab31f7#diff-4cfebe2518c34c9f3975cde19f3ced08L598

mattheworiordan · 2018-05-17T16:14:17Z

lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java

@@ -603,7 +618,10 @@ public void onChannelStateChanged(ChannelStateChange stateChange) {
 					ably.close();
 				}
 			});
+            (new Helpers.ChannelWaiter(channel)).waitFor(ChannelState.attached);


Will you please sort out your IDE/editor? It's getting tiring spending time commenting on whitespace I am afraid.

Also this is an old commit and is now fixed.

funkyboy · 2018-05-17T16:35:08Z

@mattheworiordan the build failed because there are still issues with crypto tests. Ongoing PR here. And some issues with history on realtime.

paddybyers

I think one change remaining as commented but otherwise LGTM.

funkyboy · 2018-05-18T12:22:26Z

@paddybyers done 5da11f6

…* tests

funkyboy changed the title ~~WIP: Add connection freshness check~~ Add connection freshness check May 10, 2018

funkyboy requested a review from paddybyers May 10, 2018 14:23

funkyboy force-pushed the add-connection-freshness-check branch from 11d348f to db571df Compare May 10, 2018 14:29

funkyboy mentioned this pull request May 10, 2018

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

Merged

funkyboy force-pushed the add-connectionStateTtl branch from bd36890 to 56c30f2 Compare May 10, 2018 14:57

funkyboy added 8 commits May 10, 2018 19:17

Add connection freshness check

664c5e6

Test connectionId after ttl + idle interval has passed

41b28a1

Fix reconnection test after ttl

2b15846

Test reconnection before ttl + idleInterval has passed

dbeb79a

Fix detection of connection freshness

aa24aaa

Improve connection state tests

ec4cb43

Should wait for the connection to be suspended

Add test for RTN15g3

b16bdec

Fix implementation fo connection freshness

c75d55e

and extract it into a method

funkyboy force-pushed the add-connection-freshness-check branch from db571df to c75d55e Compare May 10, 2018 17:44

paddybyers approved these changes May 10, 2018

View reviewed changes

funkyboy added 2 commits May 14, 2018 17:45

Refactor stale connection check

cc38a40

Fix test when reconnecting and connection is stale

9efc78a

paddybyers reviewed May 14, 2018

View reviewed changes

mattheworiordan requested changes May 15, 2018

View reviewed changes

funkyboy added 7 commits May 15, 2018 11:40

Fix indentation

6a2c3e4

Add log message when clearing stale connection

0a1c622

Improve after TTL test

6892ecc

Explicitly wait for channel to attach in reconnection test

0a14d26

Explicitly wait for close in reconnection test

77f3cf8

Set lastActivity from SocketTransport

b1b9085

Improve reattach test

3a868ee

funkyboy force-pushed the add-connection-freshness-check branch from e973176 to 3a868ee Compare May 15, 2018 17:30

paddybyers and others added 7 commits May 16, 2018 12:09

Fix suspendAll() (used for RTN15c3 and RTN15g3) to ensure previously …

09d76b2

…attached channels do not enter suspended when handling a discontinuity; add a private method on ConnectionManager to support tests by dropping a connection and not automatically retrying it

Add test for RTN15c3

79d490b

Merge branch 'add-rtn15c-test' into add-connection-freshness-check

f94e877

Ongoing tests

6ce56a8

Implement RTN15g3: when suppressing resume due to a stale connection,…

056edff

… ensure that the resulting (clean) connection is handled as a resume failure, triggering reattach for discontinuity of attached channels

Clean up log statements and clarify comments

6ab31f7

Add suspended channels to RTN15g3 test

0521e14

funkyboy force-pushed the add-connection-freshness-check branch from d964888 to 0521e14 Compare May 17, 2018 09:25

To kick a Travis build

3e41787

mattheworiordan approved these changes May 17, 2018

View reviewed changes

paddybyers reviewed May 18, 2018

View reviewed changes

funkyboy force-pushed the add-connection-freshness-check branch from 5da11f6 to 810cef4 Compare May 18, 2018 12:26

Use disconnectAndSuppressRetries to pause connectionManager in RTN15g…

b681cd8

…* tests

funkyboy force-pushed the add-connection-freshness-check branch from 810cef4 to b681cd8 Compare May 18, 2018 12:28

funkyboy merged commit c0a3575 into add-connectionStateTtl May 18, 2018

funkyboy deleted the add-connection-freshness-check branch May 18, 2018 12:29

paddybyers mentioned this pull request Jul 17, 2018

Implement connection state freshness check #358

Closed

		@@ -205,6 +205,7 @@ public String getHost() {
		*********************/

		public void connect() {

Add connection freshness check #390

Add connection freshness check #390

Conversation

funkyboy commented May 9, 2018 • edited Loading

funkyboy commented May 10, 2018

paddybyers 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 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

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 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

funkyboy 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paddybyers commented May 15, 2018

funkyboy commented May 17, 2018

funkyboy commented May 17, 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

funkyboy commented May 17, 2018

paddybyers left a comment

Choose a reason for hiding this comment

funkyboy commented May 18, 2018

funkyboy commented May 9, 2018 •

edited

Loading

funkyboy May 15, 2018 •

edited

Loading