From a8327d121b9eeb3b61ae63f06095d423936a5da2 Mon Sep 17 00:00:00 2001 From: Neil Fuller Date: Fri, 28 Nov 2014 17:38:53 +0000 Subject: [PATCH 1/2] Fix for a socket leak in OkHttp on Android When the preferred Android network changes from cell -> wifi or wifi -> cell the HTTP connection pool in use is abandoned to avoid reuse of connections on the old network. This was added in commit 8bced3e. The design for the connection pool was such that continuous use of the connection pool was required to clean up idle / expired connections. If a connection pool becomes idle (as when it is dereferenced on a network change) it is possible for some connections to remain in the pool indefinitely. After the preferred network change, because the old connection pool was no longer referenced the pool would be garbage collected and Android's "Strict Mode" would complain about sockets not being closed. The only existing way to avoid this was to call "evictAll()", which would have had issues when a large number of connections were returned to the pool after evictAll() was called. It also wouldn't work for SPDY connections which are shared but not reference counted, which makes knowing whether it is safe to close them difficult. The cleanModeRunnable serves two purposes: 1) While scheduled / executing, it pins the connection pool in memory to avoid it being garbage collected. 2) It continues to close connections (safely) until the pool is empty. If a connection is then added back to the pool the cleanModeRunnable is restarted. --- .../squareup/okhttp/ConnectionPoolTest.java | 203 ++++++++++++++---- .../com/squareup/okhttp/ConnectionPool.java | 164 +++++++++----- 2 files changed, 270 insertions(+), 97 deletions(-) diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/ConnectionPoolTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/ConnectionPoolTest.java index 0d1428aea433..1bff133e3ce2 100644 --- a/okhttp-tests/src/test/java/com/squareup/okhttp/ConnectionPoolTest.java +++ b/okhttp-tests/src/test/java/com/squareup/okhttp/ConnectionPoolTest.java @@ -26,6 +26,7 @@ import java.net.Proxy; import java.util.Arrays; import java.util.List; +import java.util.concurrent.Executor; import javax.net.SocketFactory; import javax.net.ssl.SSLContext; import org.junit.After; @@ -53,6 +54,7 @@ public final class ConnectionPoolTest { private InetSocketAddress httpSocketAddress; private ConnectionPool pool; + private FakeExecutor cleanupExecutor; private Connection httpA; private Connection httpB; private Connection httpC; @@ -96,6 +98,9 @@ private void setUp(int poolSize) throws Exception { Route spdyRoute = new Route(spdyAddress, Proxy.NO_PROXY, spdySocketAddress, ConnectionSpec.MODERN_TLS); pool = new ConnectionPool(poolSize, KEEP_ALIVE_DURATION_MS); + // Disable the automatic execution of the cleanup. + cleanupExecutor = new FakeExecutor(); + pool.replaceCleanupExecutorForTests(cleanupExecutor); httpA = new Connection(pool, httpRoute); httpA.connect(200, 200, 200, null); httpB = new Connection(pool, httpRoute); @@ -160,10 +165,31 @@ private void resetWithPoolSize(int poolSize) throws Exception { assertNull(recycledConnection); } + @Test public void getDoesNotScheduleCleanup() { + Connection connection = pool.get(httpAddress); + assertNull(connection); + cleanupExecutor.assertExecutionScheduled(false); + } + + @Test public void recycleSchedulesCleanup() { + cleanupExecutor.assertExecutionScheduled(false); + pool.recycle(httpA); + cleanupExecutor.assertExecutionScheduled(true); + } + + @Test public void shareSchedulesCleanup() { + cleanupExecutor.assertExecutionScheduled(false); + pool.share(spdyA); + cleanupExecutor.assertExecutionScheduled(true); + } + @Test public void poolPrefersMostRecentlyRecycled() throws Exception { pool.recycle(httpA); pool.recycle(httpB); pool.recycle(httpC); + assertPooled(pool, httpC, httpB, httpA); + + pool.performCleanup(); assertPooled(pool, httpC, httpB); } @@ -179,10 +205,18 @@ private void resetWithPoolSize(int poolSize) throws Exception { assertPooled(pool); } - @Test public void idleConnectionNotReturned() throws Exception { + @Test public void expiredConnectionNotReturned() throws Exception { pool.recycle(httpA); + + // Allow enough time to pass so that the connection is now expired. Thread.sleep(KEEP_ALIVE_DURATION_MS * 2); + + // The connection is held, but will not be returned. assertNull(pool.get(httpAddress)); + assertPooled(pool, httpA); + + // The connection must be cleaned up. + pool.performCleanup(); assertPooled(pool); } @@ -191,21 +225,35 @@ private void resetWithPoolSize(int poolSize) throws Exception { pool.recycle(httpB); pool.recycle(httpC); pool.recycle(httpD); + assertPooled(pool, httpD, httpC, httpB, httpA); + + pool.performCleanup(); assertPooled(pool, httpD, httpC); } @Test public void expiredConnectionsAreEvicted() throws Exception { pool.recycle(httpA); pool.recycle(httpB); + + // Allow enough time to pass so that the connections are now expired. Thread.sleep(2 * KEEP_ALIVE_DURATION_MS); - pool.get(spdyAddress); // Force the cleanup callable to run. + assertPooled(pool, httpB, httpA); + + // The connections must be cleaned up. + pool.performCleanup(); assertPooled(pool); } @Test public void nonAliveConnectionNotReturned() throws Exception { pool.recycle(httpA); + + // Close the connection. It is an ex-connection. It has ceased to be. httpA.getSocket().close(); + assertPooled(pool, httpA); assertNull(pool.get(httpAddress)); + + // The connection must be cleaned up. + pool.performCleanup(); assertPooled(pool); } @@ -233,6 +281,10 @@ private void resetWithPoolSize(int poolSize) throws Exception { httpA.getSocket().shutdownInput(); pool.recycle(httpA); // Should close httpA. assertTrue(httpA.getSocket().isClosed()); + + // The pool should remain empty, and there is no need to schedule a cleanup. + assertPooled(pool); + cleanupExecutor.assertExecutionScheduled(false); } @Test public void shareHttpConnectionFails() throws Exception { @@ -241,32 +293,43 @@ private void resetWithPoolSize(int poolSize) throws Exception { fail(); } catch (IllegalArgumentException expected) { } + // The pool should remain empty, and there is no need to schedule a cleanup. assertPooled(pool); + cleanupExecutor.assertExecutionScheduled(false); } @Test public void recycleSpdyConnectionDoesNothing() throws Exception { pool.recycle(spdyA); + // The pool should remain empty, and there is no need to schedule the cleanup. assertPooled(pool); + cleanupExecutor.assertExecutionScheduled(false); } @Test public void validateIdleSpdyConnectionTimeout() throws Exception { pool.share(spdyA); - Thread.sleep((int) (KEEP_ALIVE_DURATION_MS * 0.7)); - assertNull(pool.get(httpAddress)); + assertPooled(pool, spdyA); // Connection should be in the pool. + + Thread.sleep((long) (KEEP_ALIVE_DURATION_MS * 0.7)); + pool.performCleanup(); assertPooled(pool, spdyA); // Connection should still be in the pool. - Thread.sleep((int) (KEEP_ALIVE_DURATION_MS * 0.4)); - assertNull(pool.get(httpAddress)); - assertPooled(pool); + + Thread.sleep((long) (KEEP_ALIVE_DURATION_MS * 0.4)); + pool.performCleanup(); + assertPooled(pool); // Connection should have been removed. } @Test public void validateIdleHttpConnectionTimeout() throws Exception { pool.recycle(httpA); - Thread.sleep((int) (KEEP_ALIVE_DURATION_MS * 0.7)); - assertNull(pool.get(spdyAddress)); + assertPooled(pool, httpA); // Connection should be in the pool. + cleanupExecutor.assertExecutionScheduled(true); + + Thread.sleep((long) (KEEP_ALIVE_DURATION_MS * 0.7)); + pool.performCleanup(); assertPooled(pool, httpA); // Connection should still be in the pool. - Thread.sleep((int) (KEEP_ALIVE_DURATION_MS * 0.4)); - assertNull(pool.get(spdyAddress)); - assertPooled(pool); + + Thread.sleep((long) (KEEP_ALIVE_DURATION_MS * 0.4)); + pool.performCleanup(); + assertPooled(pool); // Connection should have been removed. } @Test public void maxConnections() throws IOException, InterruptedException { @@ -285,21 +348,33 @@ private void resetWithPoolSize(int poolSize) throws Exception { assertEquals(2, pool.getHttpConnectionCount()); assertEquals(0, pool.getSpdyConnectionCount()); - // http C should be added and http A should be removed. + // http C should be added pool.recycle(httpC); - Thread.sleep(50); + assertEquals(3, pool.getConnectionCount()); + assertEquals(3, pool.getHttpConnectionCount()); + assertEquals(0, pool.getSpdyConnectionCount()); + + pool.performCleanup(); + + // http A should be removed by cleanup. assertEquals(2, pool.getConnectionCount()); assertEquals(2, pool.getHttpConnectionCount()); assertEquals(0, pool.getSpdyConnectionCount()); - // spdy A should be added and http B should be removed. + // spdy A should be added pool.share(spdyA); - Thread.sleep(50); + assertEquals(3, pool.getConnectionCount()); + assertEquals(2, pool.getHttpConnectionCount()); + assertEquals(1, pool.getSpdyConnectionCount()); + + pool.performCleanup(); + + // http B should be removed by cleanup. assertEquals(2, pool.getConnectionCount()); assertEquals(1, pool.getHttpConnectionCount()); assertEquals(1, pool.getSpdyConnectionCount()); - // http C should be removed from the pool. + // http C should be returned. Connection recycledHttpConnection = pool.get(httpAddress); recycledHttpConnection.setOwner(owner); assertNotNull(recycledHttpConnection); @@ -308,7 +383,7 @@ private void resetWithPoolSize(int poolSize) throws Exception { assertEquals(0, pool.getHttpConnectionCount()); assertEquals(1, pool.getSpdyConnectionCount()); - // spdy A will be returned and kept in the pool. + // spdy A will be returned but also kept in the pool. Connection sharedSpdyConnection = pool.get(spdyAddress); assertNotNull(sharedSpdyConnection); assertEquals(spdyA, sharedSpdyConnection); @@ -316,9 +391,8 @@ private void resetWithPoolSize(int poolSize) throws Exception { assertEquals(0, pool.getHttpConnectionCount()); assertEquals(1, pool.getSpdyConnectionCount()); - // Nothing should change. + // http C should be added to the pool pool.recycle(httpC); - Thread.sleep(50); assertEquals(2, pool.getConnectionCount()); assertEquals(1, pool.getHttpConnectionCount()); assertEquals(1, pool.getSpdyConnectionCount()); @@ -331,7 +405,7 @@ private void resetWithPoolSize(int poolSize) throws Exception { assertEquals(0, pool.getHttpConnectionCount()); assertEquals(1, pool.getSpdyConnectionCount()); - // spdy A will be returned and kept in the pool. Pool shouldn't change. + // spdy A will be returned but also kept in the pool. sharedSpdyConnection = pool.get(spdyAddress); assertEquals(spdyA, sharedSpdyConnection); assertNotNull(sharedSpdyConnection); @@ -341,51 +415,52 @@ private void resetWithPoolSize(int poolSize) throws Exception { // http D should be added to the pool. pool.recycle(httpD); - Thread.sleep(50); assertEquals(2, pool.getConnectionCount()); assertEquals(1, pool.getHttpConnectionCount()); assertEquals(1, pool.getSpdyConnectionCount()); - // http E should be added to the pool. spdy A should be removed from the pool. + // http E should be added to the pool. pool.recycle(httpE); - Thread.sleep(50); + assertEquals(3, pool.getConnectionCount()); + assertEquals(2, pool.getHttpConnectionCount()); + assertEquals(1, pool.getSpdyConnectionCount()); + + pool.performCleanup(); + + // spdy A should be removed from the pool by cleanup. assertEquals(2, pool.getConnectionCount()); assertEquals(2, pool.getHttpConnectionCount()); assertEquals(0, pool.getSpdyConnectionCount()); } - @Test public void connectionCleanup() throws IOException, InterruptedException { + @Test public void connectionCleanup() throws Exception { ConnectionPool pool = new ConnectionPool(10, KEEP_ALIVE_DURATION_MS); // Add 3 connections to the pool. pool.recycle(httpA); pool.recycle(httpB); pool.share(spdyA); - assertEquals(3, pool.getConnectionCount()); - assertEquals(2, pool.getHttpConnectionCount()); - assertEquals(1, pool.getSpdyConnectionCount()); // Kill http A. Util.closeQuietly(httpA.getSocket()); - // Force pool to run a clean up. - assertNotNull(pool.get(spdyAddress)); - Thread.sleep(50); + assertEquals(3, pool.getConnectionCount()); + assertEquals(2, pool.getHttpConnectionCount()); + assertEquals(1, pool.getSpdyConnectionCount()); + // Http A should be removed. + pool.performCleanup(); + assertPooled(pool, spdyA, httpB); assertEquals(2, pool.getConnectionCount()); assertEquals(1, pool.getHttpConnectionCount()); assertEquals(1, pool.getSpdyConnectionCount()); - Thread.sleep(KEEP_ALIVE_DURATION_MS); - // Force pool to run a clean up. - assertNull(pool.get(httpAddress)); - assertNull(pool.get(spdyAddress)); - - Thread.sleep(50); + // Now let enough time pass for the connections to expire. + Thread.sleep(2 * KEEP_ALIVE_DURATION_MS); + // All remaining connections should be removed. + pool.performCleanup(); assertEquals(0, pool.getConnectionCount()); - assertEquals(0, pool.getHttpConnectionCount()); - assertEquals(0, pool.getSpdyConnectionCount()); } @Test public void evictAllConnections() throws Exception { @@ -421,7 +496,57 @@ private void resetWithPoolSize(int poolSize) throws Exception { } } + @Test public void cleanupRunnableStopsEventually() throws Exception { + pool.recycle(httpA); + pool.share(spdyA); + assertPooled(pool, spdyA, httpA); + + // The cleanup should terminate once the pool is empty again. + cleanupExecutor.fakeExecute(); + assertPooled(pool); + + cleanupExecutor.assertExecutionScheduled(false); + + // Adding a new connection should cause the cleanup to start up again. + pool.recycle(httpB); + + cleanupExecutor.assertExecutionScheduled(true); + + // The cleanup should terminate once the pool is empty again. + cleanupExecutor.fakeExecute(); + assertPooled(pool); + } + private void assertPooled(ConnectionPool pool, Connection... connections) throws Exception { assertEquals(Arrays.asList(connections), pool.getConnections()); } + + /** + * An executor that does not actually execute anything by default. See + * {@link #fakeExecute()}. + */ + private static class FakeExecutor implements Executor { + + private Runnable runnable; + + @Override + public void execute(Runnable runnable) { + // This is a bonus assertion for the invariant: At no time should two runnables be scheduled. + assertNull(this.runnable); + this.runnable = runnable; + } + + public void assertExecutionScheduled(boolean expected) { + assertEquals(expected, runnable != null); + } + + /** + * Executes the runnable. + */ + public void fakeExecute() { + Runnable toRun = this.runnable; + this.runnable = null; + toRun.run(); + } + } } diff --git a/okhttp/src/main/java/com/squareup/okhttp/ConnectionPool.java b/okhttp/src/main/java/com/squareup/okhttp/ConnectionPool.java index b52c234e45ae..eae862988d3c 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/ConnectionPool.java +++ b/okhttp/src/main/java/com/squareup/okhttp/ConnectionPool.java @@ -23,7 +23,7 @@ import java.util.LinkedList; import java.util.List; import java.util.ListIterator; -import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executor; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; @@ -78,40 +78,51 @@ public final class ConnectionPool { private final LinkedList connections = new LinkedList<>(); - /** We use a single background thread to cleanup expired connections. */ - private final ExecutorService executorService = new ThreadPoolExecutor(0, 1, - 60L, TimeUnit.SECONDS, new LinkedBlockingQueue(), - Util.threadFactory("OkHttp ConnectionPool", true)); + /** + * A background thread is used to cleanup expired connections. There will be, at most, a single + * thread running per connection pool. + * + *

A {@link ThreadPoolExecutor} is used and not a + * {@link java.util.concurrent.ScheduledThreadPoolExecutor}; ScheduledThreadPoolExecutors do not + * shrink. This executor shrinks the thread pool after a period of inactivity, and starts threads + * as needed. Delays are instead handled by the {@link #connectionsCleanupRunnable}. It is + * important that the {@link #connectionsCleanupRunnable} stops eventually, otherwise it will pin + * the thread, and thus the connection pool, in memory. + */ + private Executor executor = new ThreadPoolExecutor( + 0 /* corePoolSize */, 1 /* maximumPoolSize */, 60L /* keepAliveTime */, TimeUnit.SECONDS, + new LinkedBlockingQueue(), Util.threadFactory("OkHttp ConnectionPool", true)); + + /** {@code true} if the pool is actively draining, {@code false} if it is currently empty. */ + private boolean draining; + private final Runnable connectionsCleanupRunnable = new Runnable() { + // An executing connectionsCleanupRunnable keeps a reference to the enclosing ConnectionPool, + // preventing the ConnectionPool from being garbage collected before all held connections have + // been explicitly closed. If this was not the case any open connections in the pool would + // trigger StrictMode violations in Android when they were garbage collected. http://b/18369687 @Override public void run() { - List expiredConnections = new ArrayList<>(MAX_CONNECTIONS_TO_CLEANUP); - int idleConnectionCount = 0; - synchronized (ConnectionPool.this) { - for (ListIterator i = connections.listIterator(connections.size()); - i.hasPrevious(); ) { - Connection connection = i.previous(); - if (!connection.isAlive() || connection.isExpired(keepAliveDurationNs)) { - i.remove(); - expiredConnections.add(connection); - if (expiredConnections.size() == MAX_CONNECTIONS_TO_CLEANUP) break; - } else if (connection.isIdle()) { - idleConnectionCount++; + while (true) { + performCleanup(); + + // See whether this runnable should continue executing. + synchronized(ConnectionPool.this) { + if (connections.size() == 0) { + draining = false; + return; } } - for (ListIterator i = connections.listIterator(connections.size()); - i.hasPrevious() && idleConnectionCount > maxIdleConnections; ) { - Connection connection = i.previous(); - if (connection.isIdle()) { - expiredConnections.add(connection); - i.remove(); - --idleConnectionCount; - } + // Pause to avoid checking the pool too regularly, which would drain the battery on mobile + // devices. + try { + // Use the keep alive duration as a rough indicator of a good check interval. + long keepAliveDurationMillis = keepAliveDurationNs / (1000 * 1000); + Thread.sleep(keepAliveDurationMillis); + } catch (InterruptedException e) { + // Ignored. } } - for (Connection expiredConnection : expiredConnections) { - Util.closeQuietly(expiredConnection.getSocket()); - } } }; @@ -120,32 +131,6 @@ public ConnectionPool(int maxIdleConnections, long keepAliveDurationMs) { this.keepAliveDurationNs = keepAliveDurationMs * 1000 * 1000; } - /** - * Returns a snapshot of the connections in this pool, ordered from newest to - * oldest. Waits for the cleanup callable to run if it is currently scheduled. - */ - List getConnections() { - waitForCleanupCallableToRun(); - synchronized (this) { - return new ArrayList<>(connections); - } - } - - /** - * Blocks until the executor service has processed all currently enqueued - * jobs. - */ - private void waitForCleanupCallableToRun() { - try { - executorService.submit(new Runnable() { - @Override public void run() { - } - }).get(); - } catch (Exception e) { - throw new AssertionError(); - } - } - public static ConnectionPool getDefault() { return systemDefault; } @@ -201,9 +186,9 @@ public synchronized Connection get(Address address) { if (foundConnection != null && foundConnection.isSpdy()) { connections.addFirst(foundConnection); // Add it back after iteration. + scheduleCleanupAsRequired(); } - executorService.execute(connectionsCleanupRunnable); return foundConnection; } @@ -240,9 +225,8 @@ void recycle(Connection connection) { connections.addFirst(connection); connection.incrementRecycleCount(); connection.resetIdleStartTime(); + scheduleCleanupAsRequired(); } - - executorService.execute(connectionsCleanupRunnable); } /** @@ -251,10 +235,10 @@ void recycle(Connection connection) { */ void share(Connection connection) { if (!connection.isSpdy()) throw new IllegalArgumentException(); - executorService.execute(connectionsCleanupRunnable); if (connection.isAlive()) { synchronized (this) { connections.addFirst(connection); + scheduleCleanupAsRequired(); } } } @@ -271,4 +255,68 @@ public void evictAll() { Util.closeQuietly(connections.get(i).getSocket()); } } + + // Callers must synchronize on "this". + private void scheduleCleanupAsRequired() { + if (!draining) { + // A new connection has potentially been offered up to an empty / drained pool. + // Start the clean-up immediately. + draining = true; + executor.execute(connectionsCleanupRunnable); + } + } + + /** Performs a single round of pool cleanup. */ + // VisibleForTesting + void performCleanup() { + ListexpiredConnections = new ArrayList<>(MAX_CONNECTIONS_TO_CLEANUP); + int idleConnectionCount = 0; + synchronized (this) { + for (ListIterator i = connections.listIterator(connections.size()); + i.hasPrevious(); ) { + Connection connection = i.previous(); + if (!connection.isAlive() || connection.isExpired(keepAliveDurationNs)) { + i.remove(); + expiredConnections.add(connection); + if (expiredConnections.size() == MAX_CONNECTIONS_TO_CLEANUP) { + break; + } + } else if (connection.isIdle()) { + idleConnectionCount++; + } + } + + for (ListIterator i = connections.listIterator(connections.size()); + i.hasPrevious() && idleConnectionCount > maxIdleConnections; ) { + Connection connection = i.previous(); + if (connection.isIdle()) { + expiredConnections.add(connection); + i.remove(); + --idleConnectionCount; + } + } + } + + for (Connection expiredConnection : expiredConnections) { + Util.closeQuietly(expiredConnection.getSocket()); + } + } + + /** + * Replace the default {@link Executor} with a different one. Only use in tests. + */ + // VisibleForTesting + void replaceCleanupExecutorForTests(Executor cleanupExecutor) { + this.executor = cleanupExecutor; + } + + /** + * Returns a snapshot of the connections in this pool, ordered from newest to + * oldest. Only use in tests. + */ + List getConnections() { + synchronized (this) { + return new ArrayList<>(connections); + } + } } From 8c7c963c24c3e347fd0d0fd641c27058d4eadc2c Mon Sep 17 00:00:00 2001 From: Jesse Wilson Date: Fri, 26 Dec 2014 01:20:26 -0500 Subject: [PATCH 2/2] Fix some checkstyle issues in nfuller's ConnectionPool fix. --- .../main/java/com/squareup/okhttp/ConnectionPool.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/okhttp/src/main/java/com/squareup/okhttp/ConnectionPool.java b/okhttp/src/main/java/com/squareup/okhttp/ConnectionPool.java index 438b945316d6..0390917ad154 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/ConnectionPool.java +++ b/okhttp/src/main/java/com/squareup/okhttp/ConnectionPool.java @@ -106,7 +106,7 @@ public final class ConnectionPool { performCleanup(); // See whether this runnable should continue executing. - synchronized(ConnectionPool.this) { + synchronized (ConnectionPool.this) { if (connections.size() == 0) { draining = false; return; @@ -271,7 +271,7 @@ private void scheduleCleanupAsRequired() { /** Performs a single round of pool cleanup. */ // VisibleForTesting void performCleanup() { - ListexpiredConnections = new ArrayList<>(MAX_CONNECTIONS_TO_CLEANUP); + List expiredConnections = new ArrayList<>(MAX_CONNECTIONS_TO_CLEANUP); int idleConnectionCount = 0; synchronized (this) { for (ListIterator i = connections.listIterator(connections.size()); @@ -316,9 +316,8 @@ void replaceCleanupExecutorForTests(Executor cleanupExecutor) { * Returns a snapshot of the connections in this pool, ordered from newest to * oldest. Only use in tests. */ - List getConnections() { - synchronized (this) { - return new ArrayList<>(connections); - } + // VisibleForTesting + synchronized List getConnections() { + return new ArrayList<>(connections); } }