From ebf2dd16e6d3f273c6344fed2d0c6ae0f687d727 Mon Sep 17 00:00:00 2001 From: Tim Renouf Date: Mon, 10 Oct 2016 10:10:57 +0100 Subject: [PATCH 1/7] Move http tests into rest suite Also requires changes to Http.java itself to allow its tests to be in a different package. --- lib/src/main/java/io/ably/lib/http/Http.java | 8 ++-- .../{http => test/rest}/HttpHeaderTest.java | 4 +- .../lib/{http => test/rest}/HttpTest.java | 44 ++++++++++++++----- .../java/io/ably/lib/test/rest/RestSuite.java | 2 + 4 files changed, 41 insertions(+), 17 deletions(-) rename lib/src/test/java/io/ably/lib/{http => test/rest}/HttpHeaderTest.java (97%) rename lib/src/test/java/io/ably/lib/{http => test/rest}/HttpTest.java (91%) diff --git a/lib/src/main/java/io/ably/lib/http/Http.java b/lib/src/main/java/io/ably/lib/http/Http.java index a8ac6576c..1701e4005 100644 --- a/lib/src/main/java/io/ably/lib/http/Http.java +++ b/lib/src/main/java/io/ably/lib/http/Http.java @@ -324,7 +324,7 @@ public void finalize() { * @return * @throws AblyException */ - T ablyHttpExecute(String path, String method, Param[] headers, Param[] params, RequestBody requestBody, ResponseHandler responseHandler) throws AblyException { + public T ablyHttpExecute(String path, String method, Param[] headers, Param[] params, RequestBody requestBody, ResponseHandler responseHandler) throws AblyException { int retryCountRemaining = Hosts.isRestFallbackSupported(this.host) ? options.httpMaxRetryCount : 0; String candidateHost = this.host; URL url; @@ -355,7 +355,7 @@ T ablyHttpExecute(String path, String method, Param[] headers, Param[] param * @return * @throws AblyException */ - T httpExecute(URL url, String method, Param[] headers, RequestBody requestBody, ResponseHandler responseHandler) throws AblyException { + public T httpExecute(URL url, String method, Param[] headers, RequestBody requestBody, ResponseHandler responseHandler) throws AblyException { return httpExecuteWithRetry(url, method, headers, requestBody, responseHandler, false); } @@ -371,7 +371,7 @@ T httpExecute(URL url, String method, Param[] headers, RequestBody requestBo * @return * @throws AblyException */ - T httpExecute(URL url, Proxy proxy, String method, Param[] headers, RequestBody requestBody, boolean withCredentials, ResponseHandler responseHandler) throws AblyException { + public T httpExecute(URL url, Proxy proxy, String method, Param[] headers, RequestBody requestBody, boolean withCredentials, ResponseHandler responseHandler) throws AblyException { HttpURLConnection conn = null; try { conn = (HttpURLConnection)url.openConnection(proxy); @@ -451,7 +451,7 @@ T httpExecute(HttpURLConnection conn, String method, Param[] headers, Reques * @return * @throws AblyException */ - T httpExecuteWithRetry(URL url, String method, Param[] headers, RequestBody requestBody, ResponseHandler responseHandler, boolean allowAblyAuth) throws AblyException { + public T httpExecuteWithRetry(URL url, String method, Param[] headers, RequestBody requestBody, ResponseHandler responseHandler, boolean allowAblyAuth) throws AblyException { boolean authPending = true, renewPending = true, proxyAuthPending = true; while(true) { try { diff --git a/lib/src/test/java/io/ably/lib/http/HttpHeaderTest.java b/lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java similarity index 97% rename from lib/src/test/java/io/ably/lib/http/HttpHeaderTest.java rename to lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java index 3d68a0dfc..d6d04c0d0 100644 --- a/lib/src/test/java/io/ably/lib/http/HttpHeaderTest.java +++ b/lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java @@ -1,4 +1,4 @@ -package io.ably.lib.http; +package io.ably.lib.test.rest; import org.junit.AfterClass; import org.junit.Assert; @@ -11,6 +11,8 @@ import fi.iki.elonen.NanoHTTPD; import fi.iki.elonen.router.RouterNanoHTTPD; +import io.ably.lib.http.Http; +import io.ably.lib.http.HttpUtils; import io.ably.lib.rest.AblyRest; import io.ably.lib.rest.Channel; import io.ably.lib.test.common.Setup; diff --git a/lib/src/test/java/io/ably/lib/http/HttpTest.java b/lib/src/test/java/io/ably/lib/test/rest/HttpTest.java similarity index 91% rename from lib/src/test/java/io/ably/lib/http/HttpTest.java rename to lib/src/test/java/io/ably/lib/test/rest/HttpTest.java index f4ca6a374..cd522bd84 100644 --- a/lib/src/test/java/io/ably/lib/http/HttpTest.java +++ b/lib/src/test/java/io/ably/lib/test/rest/HttpTest.java @@ -1,8 +1,9 @@ -package io.ably.lib.http; +package io.ably.lib.test.rest; import fi.iki.elonen.NanoHTTPD; import fi.iki.elonen.router.RouterNanoHTTPD; import io.ably.lib.test.util.StatusHandler; +import io.ably.lib.http.Http; import io.ably.lib.transport.Defaults; import io.ably.lib.types.AblyException; import io.ably.lib.types.ClientOptions; @@ -19,6 +20,7 @@ import java.io.IOException; import java.net.MalformedURLException; +import java.net.Proxy; import java.net.URL; import java.util.ArrayList; import java.util.List; @@ -99,12 +101,12 @@ public void http_ably_execute_fallback() throws AblyException { List urlArgumentStack; @Override - T httpExecute(URL url, String method, Param[] headers, RequestBody requestBody, ResponseHandler responseHandler) throws AblyException { + public T httpExecute(URL url, Proxy proxy, String method, Param[] headers, RequestBody requestBody, boolean withCredentials, ResponseHandler responseHandler) throws AblyException { // Store a copy of given argument urlArgumentStack.add(url.getHost()); // Execute the original method without changing behavior - return super.httpExecute(url, method, headers, requestBody, responseHandler); + return super.httpExecute(url, proxy, method, headers, requestBody, withCredentials, responseHandler); } public Http setUrlArgumentStack(List urlArgumentStack) { @@ -171,9 +173,11 @@ public void http_execute_nofallback() throws Exception { .when(http) /* when following method is executed on {@code Http} instance */ .httpExecute( url.capture(), /* capture url arguments passed down httpExecute to assert fallback behavior executed with valid rest host */ + any(Proxy.class), /* Ignore */ anyString(), /* Ignore */ aryEq(new Param[0]), /* Ignore */ any(Http.RequestBody.class), /* Ignore */ + anyBoolean(), /* Ignore */ any(Http.ResponseHandler.class) /* Ignore */ ); @@ -193,10 +197,12 @@ public void http_execute_nofallback() throws Exception { * - and delivered expected response */ verify(http, times(1)) .httpExecute( /* Just validating call counter. Ignore following parameters */ - url.capture(), /* Ignore */ + url.capture(), /* capture url arguments passed down httpExecute to assert fallback behavior executed with valid rest host */ + any(Proxy.class), /* Ignore */ anyString(), /* Ignore */ aryEq(new Param[0]), /* Ignore */ any(Http.RequestBody.class), /* Ignore */ + anyBoolean(), /* Ignore */ any(Http.ResponseHandler.class) /* Ignore */ ); @@ -234,10 +240,12 @@ public void http_execute_singlefallback() throws Exception { doAnswer(answer) /* Behave as defined above */ .when(http) /* when following method is executed on {@code Http} instance */ .httpExecute( - url.capture(), /* capture url arguments passed down httpExecute to assert fallback behavior executed with valid fallback url */ + url.capture(), /* capture url arguments passed down httpExecute to assert fallback behavior executed with valid rest host */ + any(Proxy.class), /* Ignore */ anyString(), /* Ignore */ aryEq(new Param[0]), /* Ignore */ any(Http.RequestBody.class), /* Ignore */ + anyBoolean(), /* Ignore */ any(Http.ResponseHandler.class) /* Ignore */ ); @@ -259,10 +267,12 @@ public void http_execute_singlefallback() throws Exception { verify(http, times(2)) .httpExecute( /* Just validating call counter. Ignore following parameters */ - url.capture(), /* Ignore */ + url.capture(), /* capture url arguments passed down httpExecute to assert fallback behavior executed with valid rest host */ + any(Proxy.class), /* Ignore */ anyString(), /* Ignore */ aryEq(new Param[0]), /* Ignore */ any(Http.RequestBody.class), /* Ignore */ + anyBoolean(), /* Ignore */ any(Http.ResponseHandler.class) /* Ignore */ ); @@ -300,10 +310,12 @@ public void http_execute_multiplefallback() throws Exception { doAnswer(answer) /* Behave as defined above */ .when(http) /* when following method is executed on {@code Http} instance */ .httpExecute( - url.capture(), /* capture url arguments passed down httpExecute to assert fallback behavior executed with valid fallback url */ + url.capture(), /* capture url arguments passed down httpExecute to assert fallback behavior executed with valid rest host */ + any(Proxy.class), /* Ignore */ anyString(), /* Ignore */ aryEq(new Param[0]), /* Ignore */ any(Http.RequestBody.class), /* Ignore */ + anyBoolean(), /* Ignore */ any(Http.ResponseHandler.class) /* Ignore */ ); @@ -333,10 +345,12 @@ public void http_execute_multiplefallback() throws Exception { * Do the validation, after we completed the {@code ArgumentCaptor} related assertions */ verify(http, times(3)) .httpExecute( /* Just validating call counter. Ignore following parameters */ - url.capture(), /* Ignore */ + url.capture(), /* capture url arguments passed down httpExecute to assert fallback behavior executed with valid rest host */ + any(Proxy.class), /* Ignore */ anyString(), /* Ignore */ aryEq(new Param[0]), /* Ignore */ any(Http.RequestBody.class), /* Ignore */ + anyBoolean(), /* Ignore */ any(Http.ResponseHandler.class) /* Ignore */ ); } @@ -371,10 +385,12 @@ public void http_execute_consecutivecall() throws Exception { doAnswer(answer) /* Behave as defined above */ .when(http) /* when following method is executed on {@code Http} instance */ .httpExecute( - url.capture(), /* capture url arguments passed down httpExecute to assert fallback behavior executed with valid fallback url */ + url.capture(), /* capture url arguments passed down httpExecute to assert fallback behavior executed with valid rest host */ + any(Proxy.class), /* Ignore */ anyString(), /* Ignore */ aryEq(new Param[0]), /* Ignore */ any(Http.RequestBody.class), /* Ignore */ + anyBoolean(), /* Ignore */ any(Http.ResponseHandler.class) /* Ignore */ ); @@ -395,10 +411,12 @@ public void http_execute_consecutivecall() throws Exception { doReturn("Lorem Ipsum") /* Return some response string that we will ignore */ .when(http) /* when following method is executed on {@code Http} instance */ .httpExecute( - url.capture(), /* capture url arguments passed down httpExecute to assert http call successfully executed against `rest.ably.io` host */ + url.capture(), /* capture url arguments passed down httpExecute to assert fallback behavior executed with valid rest host */ + any(Proxy.class), /* Ignore */ anyString(), /* Ignore */ aryEq(new Param[0]), /* Ignore */ any(Http.RequestBody.class), /* Ignore */ + anyBoolean(), /* Ignore */ any(Http.ResponseHandler.class) /* Ignore */ ); @@ -445,10 +463,12 @@ public void http_execute_excessivefallback() throws AblyException { doAnswer(answer) /* Behave as defined above */ .when(http) /* when following method is executed on {@code Http} instance */ .httpExecute( - url.capture(), /* Ignore */ + url.capture(), /* capture url arguments passed down httpExecute to assert fallback behavior executed with valid rest host */ + any(Proxy.class), /* Ignore */ anyString(), /* Ignore */ aryEq(new Param[0]), /* Ignore */ any(Http.RequestBody.class), /* Ignore */ + anyBoolean(), /* Ignore */ any(Http.ResponseHandler.class) /* Ignore */ ); @@ -494,7 +514,7 @@ public void http_execute_response_50x() throws AblyException, MalformedURLExcept hfe = null; try { - http.httpExecute(url, Http.GET, new Param[0], requestBody, null); + String result = http.httpExecute(url, Http.GET, new Param[0], requestBody, null); } catch (AblyException.HostFailedException e) { hfe = e; } diff --git a/lib/src/test/java/io/ably/lib/test/rest/RestSuite.java b/lib/src/test/java/io/ably/lib/test/rest/RestSuite.java index be9e57ad3..9f91bd69e 100644 --- a/lib/src/test/java/io/ably/lib/test/rest/RestSuite.java +++ b/lib/src/test/java/io/ably/lib/test/rest/RestSuite.java @@ -13,6 +13,8 @@ @RunWith(Suite.class) @SuiteClasses({ + HttpTest.class, + HttpHeaderTest.class, RestAppStatsTest.class, RestInitTest.class, RestTimeTest.class, From ba296063ec93d07e58f4ba2b1fd455a2419cc177 Mon Sep 17 00:00:00 2001 From: Tim Renouf Date: Mon, 10 Oct 2016 11:54:27 +0100 Subject: [PATCH 2/7] Move transport tests into realtime suite Also requires a change to Defaults.java to allow its tests to be in a different package. --- lib/src/main/java/io/ably/lib/transport/Defaults.java | 2 +- .../realtime}/ConnectionManagerTest.java | 4 +++- .../ably/lib/{transport => test/realtime}/HostsTest.java | 7 +++++-- .../test/java/io/ably/lib/test/realtime/RealtimeSuite.java | 2 ++ 4 files changed, 11 insertions(+), 4 deletions(-) rename lib/src/test/java/io/ably/lib/{transport => test/realtime}/ConnectionManagerTest.java (98%) rename lib/src/test/java/io/ably/lib/{transport => test/realtime}/HostsTest.java (91%) diff --git a/lib/src/main/java/io/ably/lib/transport/Defaults.java b/lib/src/main/java/io/ably/lib/transport/Defaults.java index 79206a6a4..4890e70fa 100644 --- a/lib/src/main/java/io/ably/lib/transport/Defaults.java +++ b/lib/src/main/java/io/ably/lib/transport/Defaults.java @@ -8,7 +8,7 @@ public class Defaults { public static final int PROTOCOL_VERSION = 1; - static final List HOST_FALLBACKS = Arrays.asList("A.ably-realtime.com", "B.ably-realtime.com", "C.ably-realtime.com", "D.ably-realtime.com", "E.ably-realtime.com"); + public static final List HOST_FALLBACKS = Arrays.asList("A.ably-realtime.com", "B.ably-realtime.com", "C.ably-realtime.com", "D.ably-realtime.com", "E.ably-realtime.com"); public static final String HOST_REST = "rest.ably.io"; public static final String HOST_REALTIME = "realtime.ably.io"; public static final int PORT = 80; diff --git a/lib/src/test/java/io/ably/lib/transport/ConnectionManagerTest.java b/lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java similarity index 98% rename from lib/src/test/java/io/ably/lib/transport/ConnectionManagerTest.java rename to lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java index cfc80e089..15bb0a508 100644 --- a/lib/src/test/java/io/ably/lib/transport/ConnectionManagerTest.java +++ b/lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java @@ -1,10 +1,12 @@ -package io.ably.lib.transport; +package io.ably.lib.test.realtime; import io.ably.lib.realtime.AblyRealtime; import io.ably.lib.realtime.Connection; import io.ably.lib.realtime.ConnectionState; import io.ably.lib.test.common.Helpers; import io.ably.lib.test.common.Setup; +import io.ably.lib.transport.ConnectionManager; +import io.ably.lib.transport.Defaults; import io.ably.lib.types.AblyException; import io.ably.lib.types.ClientOptions; import org.hamcrest.Matchers; diff --git a/lib/src/test/java/io/ably/lib/transport/HostsTest.java b/lib/src/test/java/io/ably/lib/test/realtime/HostsTest.java similarity index 91% rename from lib/src/test/java/io/ably/lib/transport/HostsTest.java rename to lib/src/test/java/io/ably/lib/test/realtime/HostsTest.java index 44826955d..8710fdf0d 100644 --- a/lib/src/test/java/io/ably/lib/transport/HostsTest.java +++ b/lib/src/test/java/io/ably/lib/test/realtime/HostsTest.java @@ -1,4 +1,7 @@ -package io.ably.lib.transport; +package io.ably.lib.test.realtime; + +import io.ably.lib.transport.Defaults; +import io.ably.lib.transport.Hosts; import org.hamcrest.Matchers; import org.junit.Test; @@ -43,4 +46,4 @@ public void hosts_fallback_traverse_all() { assertThat(host, is(equalTo(null))); } -} \ No newline at end of file +} diff --git a/lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java b/lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java index dfe2aed99..cf72f74c4 100644 --- a/lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java +++ b/lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java @@ -13,6 +13,8 @@ @RunWith(Suite.class) @SuiteClasses({ + ConnectionManagerTest.class, + HostsTest.class, EventEmitterTest.class, RealtimeInitTest.class, RealtimeConnectTest.class, From 90761872f51ff773cf8f6b5def12061c18c96b83 Mon Sep 17 00:00:00 2001 From: Tim Renouf Date: Tue, 11 Oct 2016 17:27:39 +0100 Subject: [PATCH 3/7] ConnectionManager: update http host on connection success Set the http host to try and ensure that realtime and rest use the same region: - if we're on the default realtime host, set http to the default rest host - otherwise (the realtime host has been overridden or has fallen back), set http to the same as realtime. Previously this was done on connection initiation. This commit changes it to happen on successful connect. --- .../io/ably/lib/transport/ConnectionManager.java | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 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 2db7dee9a..f2811ae55 100644 --- a/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java +++ b/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java @@ -329,6 +329,18 @@ private void onChannelMessage(ProtocolMessage message) { } private synchronized void onConnected(ProtocolMessage message) { + /* Set the http host to try and ensure that realtime and rest use the + * same region: + * - if we're on the default realtime host, set http to the default + * rest host + * - otherwise (the realtime host has been overridden or has fallen + * back), set http to the same as realtime. + */ + if (pendingConnect.host == options.realtimeHost) + ably.http.setHost(options.restHost); + else + ably.http.setHost(pendingConnect.host); + /* if there was a (non-fatal) connection error * that invalidates an existing connection id, then * remove all channels attached to the previous id */ @@ -632,12 +644,10 @@ private boolean connectImpl(StateIndication request) { pendingConnect = new ConnectParams(options); pendingConnect.host = hostFallback; - ably.http.setHost(hostFallback); } else { pendingConnect = new ConnectParams(options); pendingConnect.host = options.realtimeHost; - ably.http.setHost(options.restHost); } /* enter the connecting state */ From 4adedb0413e82a1da803d100e7d6245ebad91138 Mon Sep 17 00:00:00 2001 From: Tim Renouf Date: Tue, 11 Oct 2016 17:35:20 +0100 Subject: [PATCH 4/7] ConnectionManager: get fallback working Fallback was not working after a ConnectionManager timeout. Fixed. This commit also ensures that the ConnectionManager default errors have http status codes, as required for the fallback fix. --- .../ably/lib/transport/ConnectionManager.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 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 f2811ae55..5f055ee84 100644 --- a/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java +++ b/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java @@ -34,14 +34,14 @@ public class ConnectionManager implements Runnable, ConnectListener { * default errors ***********************************/ - static ErrorInfo REASON_CLOSED = new ErrorInfo("Connection closed by client", 10000); - static ErrorInfo REASON_DISCONNECTED = new ErrorInfo("Connection temporarily unavailable", 80003); - static ErrorInfo REASON_SUSPENDED = new ErrorInfo("Connection unavailable", 80002); - static ErrorInfo REASON_FAILED = new ErrorInfo("Connection failed", 80000); - static ErrorInfo REASON_REFUSED = new ErrorInfo("Access refused", 40100); - static ErrorInfo REASON_TOO_BIG = new ErrorInfo("Connection closed; message too large", 40000); - static ErrorInfo REASON_NEVER_CONNECTED = new ErrorInfo("Unable to establish connection", 80002); - static ErrorInfo REASON_TIMEDOUT = new ErrorInfo("Unable to establish connection", 80014); + static ErrorInfo REASON_CLOSED = new ErrorInfo("Connection closed by client", 200, 10000); + static ErrorInfo REASON_DISCONNECTED = new ErrorInfo("Connection temporarily unavailable", 503, 80003); + static ErrorInfo REASON_SUSPENDED = new ErrorInfo("Connection unavailable", 503, 80002); + static ErrorInfo REASON_FAILED = new ErrorInfo("Connection failed", 503, 80000); + static ErrorInfo REASON_REFUSED = new ErrorInfo("Access refused", 401, 40100); + static ErrorInfo REASON_TOO_BIG = new ErrorInfo("Connection closed; message too large", 400, 40000); + static ErrorInfo REASON_NEVER_CONNECTED = new ErrorInfo("Unable to establish connection", 503, 80002); + static ErrorInfo REASON_TIMEDOUT = new ErrorInfo("Unable to establish connection", 503, 80014); /*********************************** * a class encapsulating information @@ -530,7 +530,7 @@ private StateIndication checkSuspend(StateIndication stateChange) { * - the suspend timer has expired, so we're going into suspended state. */ - if(pendingConnect != null && stateChange.reason == null) { + if(pendingConnect != null && (stateChange.reason == null || stateChange.reason.statusCode >= 500)) { if (!Hosts.isFallback(pendingConnect.host)) { if (!checkConnectivity()) { return new StateIndication(ConnectionState.failed, new ErrorInfo("connection failed", 80000), false, pendingConnect.host); From a9795276ece16c290cef872ff53470e518e9f3e9 Mon Sep 17 00:00:00 2001 From: Tim Renouf Date: Tue, 11 Oct 2016 17:42:58 +0100 Subject: [PATCH 5/7] ConnectionManager: fixed problems from abandoned transports I was getting problems where a transport that ConnectionManager had abandoned due to timing out was, a bit later, generating an onTransportUnavailable that was not ignored. Fixed by setting ConnectionManager.transport in connectImpl so we can tell whether events are from the right transport. This fix leads to double disconnected notifications from the transport. So I also needed to fix handleStateChange to stop a transition from suspended to disconnected. Also added a 1s delay in RealtimeConnectFailTest.connect_fail_suspended to ensure that it detects the double disconnected bug. --- .../ably/lib/transport/ConnectionManager.java | 27 +++++++++++++++---- .../realtime/RealtimeConnectFailTest.java | 6 +++++ 2 files changed, 28 insertions(+), 5 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 5f055ee84..826e7810d 100644 --- a/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java +++ b/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java @@ -224,7 +224,8 @@ synchronized void notifyState(ITransport transport, StateIndication state) { if(states.get(state.state).terminal) this.transport = null; notifyState(state); - } + } else + Log.v(TAG, "notifyState: wrong transport"); } synchronized void notifyState(StateIndication state) { @@ -507,6 +508,11 @@ private void handleStateChange(StateIndication stateChange) { /* we were connected, so retry immediately */ requestState(ConnectionState.connecting); break; + case suspended: + /* Don't allow a second disconnected to make the state come out of suspended. */ + Log.v(TAG, "handleStateChange: not moving out of suspended"); + stateChange = null; + break; default: break; } @@ -607,11 +613,15 @@ public void run() { @Override public void onTransportAvailable(ITransport transport, TransportParams params) { - this.transport = transport; } @Override public synchronized void onTransportUnavailable(ITransport transport, TransportParams params, ErrorInfo reason) { + if (this.transport != transport) { + /* This is from a transport that we have already abandoned. */ + Log.v(TAG, "onTransportUnavailable: wrong transport"); + return; + } ably.auth.onAuthError(reason); notifyState(new StateIndication(ConnectionState.disconnected, reason, false, transport.getHost())); transport = null; @@ -662,17 +672,24 @@ private boolean connectImpl(StateIndication request) { Log.e(getClass().getName(), msg, e); throw new RuntimeException(msg, e); } + ITransport oldTransport; + synchronized(this) { + oldTransport = this.transport; + this.transport = transport; + } + if (oldTransport != null) + oldTransport.abort(REASON_TIMEDOUT); transport.connect(this); - return true; } private void closeImpl(StateIndication request) { + boolean connectionExist = state.state == ConnectionState.connected; /* enter the closing state */ notifyState(request); - /* send a close message on the transport, if any */ - if(transport != null) { + /* send a close message on the transport, if connected */ + if(connectionExist) { try { transport.send(new ProtocolMessage(Action.close)); } catch (AblyException e) { diff --git a/lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java b/lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java index b80e9ca1f..86f9f4b7f 100644 --- a/lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java +++ b/lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java @@ -112,6 +112,12 @@ public void connect_fail_suspended() { ConnectionWaiter connectionWaiter = new ConnectionWaiter(ably.connection); connectionWaiter.waitFor(ConnectionState.suspended); + /* Wait 1s to force bug where it changes to disconnected right after + * changing to suspended. Without this it fails only intermittently + * when that bug is present. */ + try { + Thread.sleep(1000); + } catch (InterruptedException e) {} assertEquals("Verify suspended state is reached", ConnectionState.suspended, ably.connection.state); assertTrue("Verify multiple connect attempts", connectionWaiter.getCount(ConnectionState.connecting) > 1); assertTrue("Verify multiple connect attempts", connectionWaiter.getCount(ConnectionState.disconnected) > 1); From 63dddb2c3f3db5cd1cdc6e4216a399d6100ed3ae Mon Sep 17 00:00:00 2001 From: Tim Renouf Date: Tue, 11 Oct 2016 17:47:45 +0100 Subject: [PATCH 6/7] ConnectionManager: determine fallback earlier Previously, it was determining whether to try and fall back, and thus stay in "connecting" state, in checkSuspend, but not determining the fallback host until connectImpl. The problem with that is that failure to get a fallback host (the list has been exhausted, or the original host is non-default so no fallback is done) resulted in "failure" state. Now it determines the fallback host in checkSuspend, so it is in a position to change to "disconnected" state if that fails. This fix also ensures that lack of internet connectivity leads to "disconnected" rather than "failed". --- .../ably/lib/transport/ConnectionManager.java | 52 ++++++++----------- 1 file changed, 22 insertions(+), 30 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 826e7810d..23e48d2a7 100644 --- a/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java +++ b/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java @@ -52,14 +52,14 @@ public class ConnectionManager implements Runnable, ConnectListener { public static class StateIndication { final ConnectionState state; final ErrorInfo reason; - final boolean fallback; + final String fallback; final String currentHost; public StateIndication(ConnectionState state, ErrorInfo reason) { - this(state, reason, false, null); + this(state, reason, null, null); } - public StateIndication(ConnectionState state, ErrorInfo reason, boolean fallback, String currentHost) { + public StateIndication(ConnectionState state, ErrorInfo reason, String fallback, String currentHost) { this.state = state; this.reason = reason; this.fallback = fallback; @@ -460,7 +460,7 @@ private void handleStateRequest() { break; case connecting: if(!connectImpl(requestedState)) { - indicatedState = new StateIndication(ConnectionState.failed, new ErrorInfo("Connection failed; no host available", 404, 80000), false, requestedState.currentHost); + indicatedState = new StateIndication(ConnectionState.failed, new ErrorInfo("Connection failed; no host available", 404, 80000), null, requestedState.currentHost); } handled = true; @@ -537,17 +537,18 @@ private StateIndication checkSuspend(StateIndication stateChange) { */ if(pendingConnect != null && (stateChange.reason == null || stateChange.reason.statusCode >= 500)) { - if (!Hosts.isFallback(pendingConnect.host)) { - if (!checkConnectivity()) { - return new StateIndication(ConnectionState.failed, new ErrorInfo("connection failed", 80000), false, pendingConnect.host); + if (Hosts.isFallback(pendingConnect.host) || checkConnectivity()) { + /* we will try a fallback host */ + String hostFallback = Hosts.isRealtimeFallbackSupported(options.realtimeHost)?(Hosts.getFallback(pendingConnect.host)):(null); + if (hostFallback != null) { + Log.v(TAG, "checkSuspend: fallback to " + hostFallback); + requestState(new StateIndication(ConnectionState.connecting, null, hostFallback, pendingConnect.host)); + /* returning null ensures we stay in the connecting state */ + return null; } } - - /* we will try a fallback host */ - requestState(new StateIndication(ConnectionState.connecting, null, true, pendingConnect.host)); - /* returning null ensures we stay in the connecting state */ - return null; } + Log.v(TAG, "checkSuspend: not falling back"); boolean suspendMode = System.currentTimeMillis() > suspendTime; ConnectionState expiredState = suspendMode ? ConnectionState.suspended : ConnectionState.disconnected; return new StateIndication(expiredState, stateChange.reason); @@ -623,7 +624,7 @@ public synchronized void onTransportUnavailable(ITransport transport, TransportP return; } ably.auth.onAuthError(reason); - notifyState(new StateIndication(ConnectionState.disconnected, reason, false, transport.getHost())); + notifyState(new StateIndication(ConnectionState.disconnected, reason, null, transport.getHost())); transport = null; } @@ -642,23 +643,14 @@ private boolean connectImpl(StateIndication request) { * instance the transport. * First, choose the transport. (Right now there's only one.) * Second, choose the host. ConnectParams will use the default - * (or requested) host, unless fallback=true, in which case - * it will choose a fallback host at random */ - - if(request.fallback) { - String hostFallback = Hosts.isRealtimeFallbackSupported(options.realtimeHost)?(Hosts.getFallback(request.currentHost)):(null); - - if (hostFallback == null) { - return false; - } - - pendingConnect = new ConnectParams(options); - pendingConnect.host = hostFallback; - } - else { - pendingConnect = new ConnectParams(options); - pendingConnect.host = options.realtimeHost; - } + * (or requested) host, unless fallback!=null, in which case + * checkSuspend has already chosen a fallback host at random */ + + String host = request.fallback; + if (host == null) + host = options.realtimeHost; + pendingConnect = new ConnectParams(options); + pendingConnect.host = host; /* enter the connecting state */ notifyState(request); From aeaa07b940ac0bdee104f9bb379e53473326a43e Mon Sep 17 00:00:00 2001 From: Tim Renouf Date: Tue, 11 Oct 2016 14:29:14 +0100 Subject: [PATCH 7/7] Test fixes: ConnectionManagerTest * Connection failure due to incorrect hostname or incorrect port should lead to "disconnected" state, not "failed" state, even after fallbacks. * Lack of internet connectivity (which suppresses fallbacks) should lead to "disconnected" state, not "failed" state. * Using a sandbox application key on production realtime gives "failed". * The http host name gets updated to match the realtime one only when realtime connection succeeds, which means that none of the tests here for updating the http host can work. --- .../test/realtime/ConnectionManagerTest.java | 58 +++++++++++-------- 1 file changed, 33 insertions(+), 25 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 15bb0a508..266b47be0 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 @@ -71,13 +71,13 @@ public void connectionmanager_fallback_none_customhost() throws AblyException { AblyRealtime ably = new AblyRealtime(opts); ConnectionManager connectionManager = ably.connection.connectionManager; - new Helpers.ConnectionManagerWaiter(connectionManager).waitFor(ConnectionState.failed); + new Helpers.ConnectionManagerWaiter(connectionManager).waitFor(ConnectionState.disconnected); /* Verify that, - * - connectionManager is connected - * - connectionManager is connected to the host without any fallback + * - connectionManager is disconnected + * - connectionManager's last host did not have any fallback */ - assertThat(connectionManager.getConnectionState().state, is(ConnectionState.failed)); + assertThat(connectionManager.getConnectionState().state, is(ConnectionState.disconnected)); assertThat(connectionManager.getHost(), is(equalTo(opts.realtimeHost))); } @@ -111,13 +111,13 @@ protected boolean checkConnectivity() { connectionManager.connect(); - new Helpers.ConnectionManagerWaiter(connectionManager).waitFor(ConnectionState.failed); + new Helpers.ConnectionManagerWaiter(connectionManager).waitFor(ConnectionState.disconnected); /* Verify that, - * - connectionManager is failed - * - connectionManager is didn't applied any fallback behavior + * - connectionManager is disconnected + * - connectionManager did not apply any fallback behavior */ - assertThat(connectionManager.getConnectionState().state, is(ConnectionState.failed)); + assertThat(connectionManager.getConnectionState().state, is(ConnectionState.disconnected)); assertThat(connectionManager.getHost(), is(equalTo(opts.realtimeHost))); } @@ -145,16 +145,14 @@ public void connectionmanager_fallback_applied() throws AblyException { AblyRealtime ably = new AblyRealtime(opts); ConnectionManager connectionManager = ably.connection.connectionManager; - new Helpers.ConnectionManagerWaiter(connectionManager).waitFor(ConnectionState.failed); + new Helpers.ConnectionManagerWaiter(connectionManager).waitFor(ConnectionState.disconnected); /* Verify that, - * - connectionManager is connected - * - connectionManager is connected to a fallback host - * - Ably http client is also using the same fallback host + * - connectionManager is disconnected + * - connectionManager's last host was a fallback host */ - assertThat(connectionManager.getConnectionState().state, is(ConnectionState.failed)); + assertThat(connectionManager.getConnectionState().state, is(ConnectionState.disconnected)); assertThat(connectionManager.getHost(), is(not(equalTo(opts.realtimeHost)))); - assertThat(ably.http.getHost(), is(equalTo(connectionManager.getHost()))); } /** @@ -176,28 +174,38 @@ public void connectionmanager_reconnect_default_endpoint() throws AblyException AblyRealtime ably = new AblyRealtime(opts); ConnectionManager connectionManager = ably.connection.connectionManager; - new Helpers.ConnectionManagerWaiter(connectionManager).waitFor(ConnectionState.failed); + System.out.println("waiting for disconnected"); + new Helpers.ConnectionManagerWaiter(connectionManager).waitFor(ConnectionState.disconnected); + System.out.println("got disconnected"); /* Verify that, - * - connectionManager is connected - * - connectionManager is connected to a fallback host - * - Ably http client is also using the same fallback host + * - connectionManager is disconnected + * - connectionManager's last host was a fallback host */ - assertThat(connectionManager.getConnectionState().state, is(ConnectionState.failed)); + assertThat(connectionManager.getConnectionState().state, is(ConnectionState.disconnected)); assertThat(connectionManager.getHost(), is(not(equalTo(opts.realtimeHost)))); - assertThat(ably.http.getHost(), is(equalTo(connectionManager.getHost()))); /* Reconnect */ ably.options.tlsPort = Defaults.TLS_PORT; + System.out.println("about to connect"); ably.connection.connect(); - new Helpers.ConnectionManagerWaiter(connectionManager).waitFor(ConnectionState.connected); + /* Wait 1s and then get the name of the host it is trying to connect to. */ + try { + Thread.sleep(1000); + } catch (InterruptedException e) {} + String host = connectionManager.getHost(); + + new Helpers.ConnectionManagerWaiter(connectionManager).waitFor(ConnectionState.failed); /* Verify that, - * - connectionManager is connected - * - connectionManager is connected to the host without any fallback + * - connectionManager is failed, because we are using an application key + * that only works on sandbox; + * - connectionManager first tried to connect to the original host, not a fallback one. */ - assertThat(connectionManager.getConnectionState().state, is(ConnectionState.connected)); - assertThat(connectionManager.getHost(), is(equalTo(opts.realtimeHost))); + System.out.println("waiting for failed"); + assertThat(connectionManager.getConnectionState().state, is(ConnectionState.failed)); + System.out.println("got failed"); + assertThat(host, is(equalTo(opts.realtimeHost))); } }