Add connection freshness check #390
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,8 +61,3 @@ notifications: | |
| - [email protected] | ||
| on_success: change | ||
| on_failure: always | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -276,7 +276,7 @@ private void setState(StateIndication newState) { | |
| /* (RTL3c) If the connection state enters the SUSPENDED | ||
| * state, then an ATTACHING or ATTACHED channel state | ||
| * will transition to SUSPENDED. */ | ||
| channel.setSuspended(state.defaultErrorInfo, true); | ||
| break; | ||
| } | ||
| } | ||
|
|
@@ -508,7 +508,7 @@ private void onChannelMessage(ProtocolMessage message) { | |
| if (connection.key != null) | ||
| connection.recoveryKey = connection.key + ":" + message.connectionSerial; | ||
| } | ||
| ably.channels.onChannelMessage(transport, message); | ||
| } | ||
|
|
||
| private synchronized void onConnected(ProtocolMessage message) { | ||
|
|
@@ -519,17 +519,30 @@ private synchronized void onConnected(ProtocolMessage message) { | |
| * - otherwise (the realtime host has been overridden or has fallen | ||
| * back), set http to the same as realtime. | ||
| */ | ||
| if (pendingConnect.host == options.realtimeHost) { | ||
| ably.httpCore.setHost(options.restHost); | ||
| } else { | ||
| ably.httpCore.setHost(pendingConnect.host); | ||
| } | ||
|
|
||
| /* if the returned connection id differs from | ||
| * the existing connection id, then this means | ||
| * we need to suspend all existing attachments to | ||
| * the old connection. | ||
| * If realtime did not reply with an error, it | ||
| * signifies that this was a result of an earlier | ||
| * connection being invalidated due to being stale. | ||
| * | ||
| * Suspend all channels attached to the previous id; | ||
| * this will be reattached in setConnection() */ | ||
| ErrorInfo error = message.error; | ||
| if(connection.id != null && !message.connectionId.equals(connection.id)) { | ||
| /* we need to suspend the original connection */ | ||
| if(error == null) { | ||
| error = REASON_SUSPENDED; | ||
| } | ||
| ably.channels.suspendAll(error, false); | ||
| } | ||
|
|
||
| /* set the new connection id */ | ||
| ConnectionDetails connectionDetails = message.connectionDetails; | ||
|
|
@@ -669,12 +682,37 @@ private void handleStateRequest() { | |
| requestedState = null; | ||
| } | ||
|
|
||
| private boolean checkConnectionStale() { | ||
| if(lastActivity == 0) { | ||
| return false; | ||
| } | ||
| long now = System.currentTimeMillis(); | ||
| long intervalSinceLastActivity = now - lastActivity; | ||
| if(intervalSinceLastActivity > (maxIdleInterval + connectionStateTtl)) { | ||
| /* RTN15g1, RTN15g2 Force a new connection if the previous one is stale; | ||
| * Clearing connection.key will ensure that we don't attempt to resume; | ||
| * leaving the original connection.id will mean that we notice at | ||
| * connection time that the connectionId has changed */ | ||
| if(connection.key != null) { | ||
| Log.v(TAG, "Clearing stale connection key to suppress resume"); | ||
| connection.key = null; | ||
| connection.recoveryKey = null; | ||
| } | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| 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()) { | ||
|
There was a problem hiding this comment. Appears to be typos in the comments above. There was a problem hiding this comment. @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. There was a problem hiding this comment. Well I misunderstood that |
||
| requestState(ConnectionState.suspended); | ||
|
There was a problem hiding this comment. Should we not add a There was a problem hiding this comment. It would be useful in future to know why a connection moved suddenly to suspended There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. There was a problem hiding this comment. @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 |
||
| return; | ||
| } | ||
| switch(state.state) { | ||
| case connecting: | ||
| stateChange = checkSuspend(stateChange); | ||
|
|
@@ -692,9 +730,11 @@ private void handleStateChange(StateIndication stateChange) { | |
| stateChange = null; | ||
| break; | ||
| case connected: | ||
| setSuspendTime(); | ||
| /* we were connected, so retry immediately */ | ||
| if(!suppressRetry) { | ||
| requestState(ConnectionState.connecting); | ||
| } | ||
| break; | ||
| case suspended: | ||
| /* Don't allow a second disconnected to make the state come out of suspended. */ | ||
|
|
@@ -800,7 +840,7 @@ public void run() { | |
| } | ||
|
|
||
| /* if our state wants us to retry on timer expiry, do that */ | ||
| if(state.retry && !suppressRetry) { | ||
| requestState(ConnectionState.connecting); | ||
| continue; | ||
| } | ||
|
|
@@ -903,6 +943,7 @@ private boolean connectImpl(StateIndication request) { | |
| String host = request.fallback; | ||
| if (host == null) | ||
| host = hosts.getHost(); | ||
| checkConnectionStale(); | ||
| pendingConnect = new ConnectParams(options); | ||
| pendingConnect.host = host; | ||
| lastUsedHost = host; | ||
|
|
@@ -970,6 +1011,10 @@ protected boolean checkConnectivity() { | |
| } | ||
| } | ||
|
|
||
| protected void setLastActivity(long lastActivityTime) { | ||
| this.lastActivity = lastActivityTime; | ||
| } | ||
|
|
||
| /****************** | ||
| * event queueing | ||
| ******************/ | ||
|
|
@@ -1176,6 +1221,15 @@ public synchronized void reset(long oldMsgSerial, ErrorInfo err) { | |
|
|
||
| } | ||
|
|
||
| /******************* | ||
| * for tests only | ||
| ******************/ | ||
|
|
||
| void disconnectAndSuppressRetries() { | ||
| requestState(ConnectionState.disconnected); | ||
| suppressRetry = true; | ||
| } | ||
|
|
||
| /******************* | ||
| * internal | ||
| ******************/ | ||
|
|
@@ -1210,9 +1264,11 @@ private boolean isFatalError(ErrorInfo err) { | |
| private StateIndication indicatedState, requestedState; | ||
| private ConnectParams pendingConnect; | ||
| private boolean pendingReauth; | ||
| private boolean suppressRetry; /* for tests only; modified via reflection */ | ||
| private ITransport transport; | ||
| private long suspendTime; | ||
| private long msgSerial; | ||
| private long lastActivity; | ||
|
|
||
| /* for debug/test only */ | ||
| private RawProtocolListener protocolListener; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,4 +117,5 @@ public static interface ConnectListener { | |
| public String getURL(); | ||
|
|
||
| public String getHost(); | ||
|
|
||
| } | ||
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 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 beisConnectionStale. 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?