From 658095d354aae88eafed1f492d24a663d8cce935 Mon Sep 17 00:00:00 2001 From: Owen Parry Date: Wed, 28 May 2014 17:37:04 -0700 Subject: [PATCH 1/4] Fix RestartableHttpClient.It kills its connectionmanager when it restarts, so we have to create a new one. So don't allow passing in a single instance of connectionmanager; it won't work. Instead, create a new one with the given schemeregistry, which can now be provided - this is the main thing to customize in a connectionmanager anyways --- .../src/main/java/com/twitter/hbc/ClientBuilder.java | 12 +++++++----- .../java/com/twitter/hbc/httpclient/BasicClient.java | 8 +++++--- .../hbc/httpclient/RestartableHttpClient.java | 10 ++++++---- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/hbc-core/src/main/java/com/twitter/hbc/ClientBuilder.java b/hbc-core/src/main/java/com/twitter/hbc/ClientBuilder.java index ba3c9afc..2d6417aa 100644 --- a/hbc-core/src/main/java/com/twitter/hbc/ClientBuilder.java +++ b/hbc-core/src/main/java/com/twitter/hbc/ClientBuilder.java @@ -25,7 +25,9 @@ import com.twitter.hbc.httpclient.auth.Authentication; import org.apache.http.HttpVersion; import org.apache.http.conn.ClientConnectionManager; +import org.apache.http.conn.scheme.SchemeRegistry; import org.apache.http.impl.conn.PoolingClientConnectionManager; +import org.apache.http.impl.conn.SchemeRegistryFactory; import org.apache.http.params.BasicHttpParams; import org.apache.http.params.HttpConnectionParams; import org.apache.http.params.HttpParams; @@ -55,7 +57,7 @@ public class ClientBuilder { protected ReconnectionManager reconnectionManager; protected int socketTimeoutMillis; protected int connectionTimeoutMillis; - protected ClientConnectionManager connectionManager; + protected SchemeRegistry schemeRegistry; private static String loadVersion() { String userAgent = "Hosebird-Client"; @@ -99,7 +101,7 @@ public ClientBuilder() { socketTimeoutMillis = 60000; connectionTimeoutMillis = 4000; - connectionManager = new PoolingClientConnectionManager(); + schemeRegistry = SchemeRegistryFactory.createDefault(); } /** @@ -188,8 +190,8 @@ public ClientBuilder rateTracker(RateTracker rateTracker) { return this; } - public ClientBuilder connectionManager(ClientConnectionManager connectionManager) { - this.connectionManager = Preconditions.checkNotNull(connectionManager); + public ClientBuilder schemeRegistry(SchemeRegistry schemeRegistry) { + this.schemeRegistry = Preconditions.checkNotNull(schemeRegistry); return this; } @@ -200,7 +202,7 @@ public BasicClient build() { HttpConnectionParams.setSoTimeout(params, socketTimeoutMillis); HttpConnectionParams.setConnectionTimeout(params, connectionTimeoutMillis); return new BasicClient(name, hosts, endpoint, auth, enableGZip, processor, reconnectionManager, - rateTracker, executorService, eventQueue, params, connectionManager); + rateTracker, executorService, eventQueue, params, schemeRegistry); } } diff --git a/hbc-core/src/main/java/com/twitter/hbc/httpclient/BasicClient.java b/hbc-core/src/main/java/com/twitter/hbc/httpclient/BasicClient.java index f480444c..287416d7 100644 --- a/hbc-core/src/main/java/com/twitter/hbc/httpclient/BasicClient.java +++ b/hbc-core/src/main/java/com/twitter/hbc/httpclient/BasicClient.java @@ -26,7 +26,9 @@ import com.twitter.hbc.httpclient.auth.Authentication; import org.apache.http.client.HttpClient; import org.apache.http.conn.ClientConnectionManager; +import org.apache.http.conn.scheme.SchemeRegistry; import org.apache.http.impl.client.DefaultHttpClient; +import org.apache.http.impl.conn.PoolingClientConnectionManager; import org.apache.http.params.HttpParams; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -53,13 +55,13 @@ public class BasicClient implements Client { public BasicClient(String name, Hosts hosts, StreamingEndpoint endpoint, Authentication auth, boolean enableGZip, HosebirdMessageProcessor processor, ReconnectionManager reconnectionManager, RateTracker rateTracker, ExecutorService executorService, - @Nullable BlockingQueue eventsQueue, HttpParams params, ClientConnectionManager connectionManager) { + @Nullable BlockingQueue eventsQueue, HttpParams params, SchemeRegistry schemeRegistry) { Preconditions.checkNotNull(auth); HttpClient client; if (enableGZip) { - client = new RestartableHttpClient(auth, enableGZip, params, connectionManager); + client = new RestartableHttpClient(auth, enableGZip, params, schemeRegistry); } else { - DefaultHttpClient defaultClient = new DefaultHttpClient(connectionManager, params); + DefaultHttpClient defaultClient = new DefaultHttpClient(new PoolingClientConnectionManager(schemeRegistry), params); /** Set auth **/ auth.setupConnection(defaultClient); diff --git a/hbc-core/src/main/java/com/twitter/hbc/httpclient/RestartableHttpClient.java b/hbc-core/src/main/java/com/twitter/hbc/httpclient/RestartableHttpClient.java index 6dba3faa..5eb7b607 100644 --- a/hbc-core/src/main/java/com/twitter/hbc/httpclient/RestartableHttpClient.java +++ b/hbc-core/src/main/java/com/twitter/hbc/httpclient/RestartableHttpClient.java @@ -21,8 +21,10 @@ import org.apache.http.client.ResponseHandler; import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.conn.ClientConnectionManager; +import org.apache.http.conn.scheme.SchemeRegistry; import org.apache.http.impl.client.DecompressingHttpClient; import org.apache.http.impl.client.DefaultHttpClient; +import org.apache.http.impl.conn.PoolingClientConnectionManager; import org.apache.http.params.HttpParams; import org.apache.http.protocol.HttpContext; @@ -39,19 +41,19 @@ public class RestartableHttpClient implements HttpClient { private final Authentication auth; private final HttpParams params; private final boolean enableGZip; - private final ClientConnectionManager connectionManager; + private final SchemeRegistry schemeRegistry; - public RestartableHttpClient(Authentication auth, boolean enableGZip, HttpParams params, ClientConnectionManager connectionManager) { + public RestartableHttpClient(Authentication auth, boolean enableGZip, HttpParams params, SchemeRegistry schemeRegistry) { this.auth = Preconditions.checkNotNull(auth); this.enableGZip = enableGZip; this.params = Preconditions.checkNotNull(params); - this.connectionManager = Preconditions.checkNotNull(connectionManager); + this.schemeRegistry = Preconditions.checkNotNull(schemeRegistry); this.underlying = new AtomicReference(); } public void setup() { - DefaultHttpClient defaultClient = new DefaultHttpClient(connectionManager, params); + DefaultHttpClient defaultClient = new DefaultHttpClient(new PoolingClientConnectionManager(schemeRegistry), params); auth.setupConnection(defaultClient); From d4661a5a0f21c86c99196d0b0e73864e5f1674e9 Mon Sep 17 00:00:00 2001 From: Owen Parry Date: Thu, 29 May 2014 15:48:12 -0700 Subject: [PATCH 2/4] test --- .../httpclient/RestartableHttpClientTest.java | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 hbc-core/src/test/java/com/twitter/hbc/httpclient/RestartableHttpClientTest.java diff --git a/hbc-core/src/test/java/com/twitter/hbc/httpclient/RestartableHttpClientTest.java b/hbc-core/src/test/java/com/twitter/hbc/httpclient/RestartableHttpClientTest.java new file mode 100644 index 00000000..2d914d75 --- /dev/null +++ b/hbc-core/src/test/java/com/twitter/hbc/httpclient/RestartableHttpClientTest.java @@ -0,0 +1,46 @@ +package com.twitter.hbc.httpclient; + +import com.twitter.hbc.httpclient.auth.Authentication; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.client.methods.HttpUriRequest; +import org.apache.http.conn.scheme.SchemeRegistry; +import org.apache.http.impl.conn.SchemeRegistryFactory; +import org.apache.http.params.HttpParams; +import org.junit.Before; +import org.junit.Test; +import sun.net.www.protocol.https.HttpsURLConnectionImpl; + +import java.net.URI; +import java.net.UnknownHostException; + +import static org.mockito.Mockito.mock; + +/** + * Created by oparry on 5/29/14. + */ +public class RestartableHttpClientTest { + private Authentication mockAuth; + private SchemeRegistry defaultSchemeRegistry; + private HttpParams mockParams; + private HttpUriRequest request; + + @Before + public void setup() throws Exception { + mockAuth = mock(Authentication.class); + mockParams = mock(HttpParams.class); + defaultSchemeRegistry = SchemeRegistryFactory.createDefault(); + request = new HttpGet("http://hi"); + } + + @Test + public void testRestart() throws Exception { + RestartableHttpClient client = new RestartableHttpClient(mockAuth, true, mockParams, defaultSchemeRegistry); + client.setup(); + client.restart(); + try { + client.execute(request); // used to crash, https://github.com/twitter/hbc/issues/113 + } catch (UnknownHostException e) { + // expected + } + } +} From dea9f911ff74759a44ade2544b0f682bc46438d4 Mon Sep 17 00:00:00 2001 From: Owen Parry Date: Thu, 29 May 2014 15:52:25 -0700 Subject: [PATCH 3/4] imports --- hbc-core/src/main/java/com/twitter/hbc/ClientBuilder.java | 2 -- .../src/main/java/com/twitter/hbc/httpclient/BasicClient.java | 1 - 2 files changed, 3 deletions(-) diff --git a/hbc-core/src/main/java/com/twitter/hbc/ClientBuilder.java b/hbc-core/src/main/java/com/twitter/hbc/ClientBuilder.java index 2d6417aa..5cb27ea5 100644 --- a/hbc-core/src/main/java/com/twitter/hbc/ClientBuilder.java +++ b/hbc-core/src/main/java/com/twitter/hbc/ClientBuilder.java @@ -24,9 +24,7 @@ import com.twitter.hbc.httpclient.BasicClient; import com.twitter.hbc.httpclient.auth.Authentication; import org.apache.http.HttpVersion; -import org.apache.http.conn.ClientConnectionManager; import org.apache.http.conn.scheme.SchemeRegistry; -import org.apache.http.impl.conn.PoolingClientConnectionManager; import org.apache.http.impl.conn.SchemeRegistryFactory; import org.apache.http.params.BasicHttpParams; import org.apache.http.params.HttpConnectionParams; diff --git a/hbc-core/src/main/java/com/twitter/hbc/httpclient/BasicClient.java b/hbc-core/src/main/java/com/twitter/hbc/httpclient/BasicClient.java index 287416d7..222b76a9 100644 --- a/hbc-core/src/main/java/com/twitter/hbc/httpclient/BasicClient.java +++ b/hbc-core/src/main/java/com/twitter/hbc/httpclient/BasicClient.java @@ -25,7 +25,6 @@ import com.twitter.hbc.core.processor.HosebirdMessageProcessor; import com.twitter.hbc.httpclient.auth.Authentication; import org.apache.http.client.HttpClient; -import org.apache.http.conn.ClientConnectionManager; import org.apache.http.conn.scheme.SchemeRegistry; import org.apache.http.impl.client.DefaultHttpClient; import org.apache.http.impl.conn.PoolingClientConnectionManager; From 7358daf9af7ef4911b3a56930ed9d9d79ed451c1 Mon Sep 17 00:00:00 2001 From: Owen Parry Date: Fri, 30 May 2014 13:08:52 -0700 Subject: [PATCH 4/4] cleanup --- .../hbc/httpclient/RestartableHttpClientTest.java | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/hbc-core/src/test/java/com/twitter/hbc/httpclient/RestartableHttpClientTest.java b/hbc-core/src/test/java/com/twitter/hbc/httpclient/RestartableHttpClientTest.java index 2d914d75..4111aaff 100644 --- a/hbc-core/src/test/java/com/twitter/hbc/httpclient/RestartableHttpClientTest.java +++ b/hbc-core/src/test/java/com/twitter/hbc/httpclient/RestartableHttpClientTest.java @@ -1,6 +1,7 @@ package com.twitter.hbc.httpclient; import com.twitter.hbc.httpclient.auth.Authentication; +import java.net.UnknownHostException; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.conn.scheme.SchemeRegistry; @@ -8,16 +9,9 @@ import org.apache.http.params.HttpParams; import org.junit.Before; import org.junit.Test; -import sun.net.www.protocol.https.HttpsURLConnectionImpl; - -import java.net.URI; -import java.net.UnknownHostException; - +import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; -/** - * Created by oparry on 5/29/14. - */ public class RestartableHttpClientTest { private Authentication mockAuth; private SchemeRegistry defaultSchemeRegistry; @@ -39,6 +33,7 @@ public void testRestart() throws Exception { client.restart(); try { client.execute(request); // used to crash, https://github.com/twitter/hbc/issues/113 + fail("should not reach here"); } catch (UnknownHostException e) { // expected }