From baa76d6fc65d473a2c883bca0d6abe86358d9e4e Mon Sep 17 00:00:00 2001 From: evgeny Date: Fri, 14 Jun 2024 14:33:41 +0100 Subject: [PATCH 1/2] [ECO-4820] fix(ConnectionManager): update the connection close implementation to follow RTN12f When `CONNECTING` state, moves immediately to `CLOSING` we wait `CONNECTED` protocol message before sending `CLOSE`. --- .../ably/lib/transport/ConnectionManager.java | 46 ++++++++++++++----- .../test/realtime/ConnectionManagerTest.java | 16 +++++++ 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java b/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java index d6bcf199a..bb2033e42 100644 --- a/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java +++ b/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java @@ -388,8 +388,9 @@ StateIndication onTimeout() { @Override void enact(StateIndication stateIndication, ConnectionStateChange change) { super.enact(stateIndication, change); - boolean closed = closeImpl(); - if(closed) { + boolean shouldAwaitConnection = change.previous == ConnectionState.connecting; + boolean closed = closeImpl(shouldAwaitConnection); + if (closed) { addAction(new AsynchronousStateChangeAction(ConnectionState.closed)); } } @@ -1160,7 +1161,13 @@ public void onMessage(ITransport transport, ProtocolMessage message) throws Ably } break; case connected: - onConnected(message); + if (currentState.state == ConnectionState.closing) { + // Based on RTN12f, if a connected protocol message comes while in the closing state, + // send a close protocol message. + if (!trySendCloseProtocolMessage()) requestState(ConnectionState.closed); + } else { + onConnected(message); + } break; case disconnect: case disconnected: @@ -1452,6 +1459,12 @@ public synchronized void onTransportUnavailable(ITransport transport, ErrorInfo setSuspendTime(); } + // Do not fallback for closing + if (currentState.state == ConnectionState.closing) { + requestState(ConnectionState.closed); + return; + } + /* if this is a failure of a pending connection attempt, decide whether or not to attempt a fallback host */ StateIndication fallbackAttempt = checkFallback(reason); if(fallbackAttempt != null) { @@ -1524,27 +1537,38 @@ private void connectImpl(StateIndication request) { /** * Close any existing transport + * @param shouldAwaitConnection true if `CONNECTING` state, moves immediately to `CLOSING` * @return closed if true, otherwise awaiting closed indication */ - private boolean closeImpl() { - if(transport == null) { + private boolean closeImpl(boolean shouldAwaitConnection) { + if (transport == null) { return true; } + // Based on RTN12f we need to wait until connected protocol message come + if (shouldAwaitConnection) { + return false; + } + + return !trySendCloseProtocolMessage(); + } + + /** + * @return true if we successfully send `close` protocol message, false otherwise + */ + private boolean trySendCloseProtocolMessage() { try { Log.v(TAG, "Requesting connection close"); transport.send(new ProtocolMessage(ProtocolMessage.Action.close)); - return false; + return true; } catch (AblyException e) { /* we're closing, and the attempt to send the CLOSE message failed; * continue, because we're not going to reinstate the transport * just to send a CLOSE message */ + Log.v(TAG, "Closing incomplete transport"); + clearTransport(); + return false; } - - /* just close the transport */ - Log.v(TAG, "Closing incomplete transport"); - clearTransport(); - return true; } private void clearTransport() { diff --git a/lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java b/lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java index 292c96049..1b3f459f9 100644 --- a/lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java +++ b/lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java @@ -396,6 +396,22 @@ public void onConnectionStateChanged(ConnectionStateChange state) { assertEquals("Verify cm thread has exited", cmThreadState, Thread.State.TERMINATED); } + /** + * (RTN12f) Close while in connecting state + */ + @Test + public void connectionmanager_close_while_connecting() throws AblyException { + ClientOptions opts = createOptions(testVars.keys[0].keyStr); + final AblyRealtime ably = new AblyRealtime(opts); + ably.close(); + + new Helpers.ConnectionWaiter(ably.connection).waitFor(ConnectionState.closed); + ConnectionManager connectionManager = ably.connection.connectionManager; + + assertThat("connectionManager is closed", connectionManager.getConnectionState().state, is(ConnectionState.closed)); + assertThat("fallback hasn't been invoked", connectionManager.getHost(), is(equalTo(opts.environment + "-realtime.ably.io"))); + } + /** * Connect, and then perform a close(); * verify that the closed state is reached, and immediately From 478e5247fcf6d835a2ce9f5aa32f5522b0fbd676 Mon Sep 17 00:00:00 2001 From: Evgeny Khokhlov Date: Mon, 17 Jun 2024 10:50:40 +0100 Subject: [PATCH 2/2] [ECO-4820] chore(tests): add more checks Co-authored-by: sachin shinde --- .../ably/lib/test/realtime/ConnectionManagerTest.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java b/lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java index 1b3f459f9..383270153 100644 --- a/lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java +++ b/lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java @@ -403,12 +403,15 @@ public void onConnectionStateChanged(ConnectionStateChange state) { public void connectionmanager_close_while_connecting() throws AblyException { ClientOptions opts = createOptions(testVars.keys[0].keyStr); final AblyRealtime ably = new AblyRealtime(opts); - ably.close(); - - new Helpers.ConnectionWaiter(ably.connection).waitFor(ConnectionState.closed); + ConnectionWaiter connectionWaiter = new ConnectionWaiter(ably.connection); ConnectionManager connectionManager = ably.connection.connectionManager; + ably.close(); - assertThat("connectionManager is closed", connectionManager.getConnectionState().state, is(ConnectionState.closed)); + connectionWaiter.waitFor(ConnectionState.closed); + assertEquals("Previous state was closing", ConnectionState.closing, connectionWaiter.lastStateChange().previous); + assertEquals(1 , connectionWaiter.getCount(ConnectionState.connecting)); + assertEquals(0 , connectionWaiter.getCount(ConnectionState.connected)); + assertEquals("Verify closed state is reached", ConnectionState.closed, ably.connection.state); assertThat("fallback hasn't been invoked", connectionManager.getHost(), is(equalTo(opts.environment + "-realtime.ably.io"))); }