diff --git a/jetty-spdy/spdy-core/src/main/java/org/eclipse/jetty/spdy/Promise.java b/jetty-spdy/spdy-core/src/main/java/org/eclipse/jetty/spdy/Promise.java index ef3eee17d1e..d44da62dee8 100644 --- a/jetty-spdy/spdy-core/src/main/java/org/eclipse/jetty/spdy/Promise.java +++ b/jetty-spdy/spdy-core/src/main/java/org/eclipse/jetty/spdy/Promise.java @@ -80,7 +80,9 @@ public class Promise implements Handler, Future @Override public T get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException { - latch.await(timeout, unit); + boolean elapsed = !latch.await(timeout, unit); + if (elapsed) + throw new TimeoutException(); return result(); } diff --git a/jetty-spdy/spdy-jetty/src/main/java/org/eclipse/jetty/spdy/SPDYClient.java b/jetty-spdy/spdy-jetty/src/main/java/org/eclipse/jetty/spdy/SPDYClient.java index 818cd8572cd..438e2e3cf03 100644 --- a/jetty-spdy/spdy-jetty/src/main/java/org/eclipse/jetty/spdy/SPDYClient.java +++ b/jetty-spdy/spdy-jetty/src/main/java/org/eclipse/jetty/spdy/SPDYClient.java @@ -38,6 +38,7 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.atomic.AtomicReference; import javax.net.ssl.SSLEngine; import javax.net.ssl.SSLException; @@ -313,7 +314,7 @@ public class SPDYClient } @Override - public AsyncConnection newConnection(final SocketChannel channel, AsyncEndPoint endPoint, final Object attachment) + public AsyncConnection newConnection(final SocketChannel channel, AsyncEndPoint endPoint, Object attachment) { SessionPromise sessionPromise = (SessionPromise)attachment; final SPDYClient client = sessionPromise.client; @@ -322,11 +323,31 @@ public class SPDYClient { if (sslContextFactory != null) { + final AtomicReference sslEndPointRef = new AtomicReference<>(); + final AtomicReference attachmentRef = new AtomicReference<>(attachment); SSLEngine engine = client.newSSLEngine(sslContextFactory, channel); - SslConnection sslConnection = new SslConnection(engine, endPoint); + SslConnection sslConnection = new SslConnection(engine, endPoint) + { + @Override + public void onClose() + { + sslEndPointRef.set(null); + attachmentRef.set(null); + super.onClose(); + } + }; endPoint.setConnection(sslConnection); - final AsyncEndPoint sslEndPoint = sslConnection.getSslEndPoint(); + AsyncEndPoint sslEndPoint = sslConnection.getSslEndPoint(); + sslEndPointRef.set(sslEndPoint); + // Instances of the ClientProvider inner class strong reference the + // SslEndPoint (via lexical scoping), which strong references the SSLEngine. + // Since NextProtoNego stores in a WeakHashMap the SSLEngine as key + // and this instance as value, we are in the situation where the value + // of a WeakHashMap refers indirectly to the key, which is bad because + // the entry will never be removed from the WeakHashMap. + // We use AtomicReferences to be captured via lexical scoping, + // and we null them out above when the connection is closed. NextProtoNego.put(engine, new NextProtoNego.ClientProvider() { @Override @@ -340,7 +361,8 @@ public class SPDYClient { // Server does not support NPN, but this is a SPDY client, so hardcode SPDY ClientSPDYAsyncConnectionFactory connectionFactory = new ClientSPDYAsyncConnectionFactory(); - AsyncConnection connection = connectionFactory.newAsyncConnection(channel, sslEndPoint, attachment); + AsyncEndPoint sslEndPoint = sslEndPointRef.get(); + AsyncConnection connection = connectionFactory.newAsyncConnection(channel, sslEndPoint, attachmentRef.get()); sslEndPoint.setConnection(connection); } @@ -352,9 +374,9 @@ public class SPDYClient return null; AsyncConnectionFactory connectionFactory = client.getAsyncConnectionFactory(protocol); - AsyncConnection connection = connectionFactory.newAsyncConnection(channel, sslEndPoint, attachment); + AsyncEndPoint sslEndPoint = sslEndPointRef.get(); + AsyncConnection connection = connectionFactory.newAsyncConnection(channel, sslEndPoint, attachmentRef.get()); sslEndPoint.setConnection(connection); - return protocol; } }); diff --git a/jetty-spdy/spdy-jetty/src/main/java/org/eclipse/jetty/spdy/SPDYServerConnector.java b/jetty-spdy/spdy-jetty/src/main/java/org/eclipse/jetty/spdy/SPDYServerConnector.java index 6e5f913d69c..2a496070f96 100644 --- a/jetty-spdy/spdy-jetty/src/main/java/org/eclipse/jetty/spdy/SPDYServerConnector.java +++ b/jetty-spdy/spdy-jetty/src/main/java/org/eclipse/jetty/spdy/SPDYServerConnector.java @@ -29,6 +29,7 @@ import java.util.concurrent.Executor; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import javax.net.ssl.SSLEngine; import javax.net.ssl.SSLException; @@ -176,16 +177,35 @@ public class SPDYServerConnector extends SelectChannelConnector if (sslContextFactory != null) { SSLEngine engine = newSSLEngine(sslContextFactory, channel); - SslConnection sslConnection = new SslConnection(engine, endPoint); + final AtomicReference sslEndPointRef = new AtomicReference<>(); + SslConnection sslConnection = new SslConnection(engine, endPoint) + { + @Override + public void onClose() + { + sslEndPointRef.set(null); + super.onClose(); + } + }; endPoint.setConnection(sslConnection); - final AsyncEndPoint sslEndPoint = sslConnection.getSslEndPoint(); + AsyncEndPoint sslEndPoint = sslConnection.getSslEndPoint(); + sslEndPointRef.set(sslEndPoint); + // Instances of the ServerProvider inner class strong reference the + // SslEndPoint (via lexical scoping), which strong references the SSLEngine. + // Since NextProtoNego stores in a WeakHashMap the SSLEngine as key + // and this instance as value, we are in the situation where the value + // of a WeakHashMap refers indirectly to the key, which is bad because + // the entry will never be removed from the WeakHashMap. + // We use AtomicReferences to be captured via lexical scoping, + // and we null them out above when the connection is closed. NextProtoNego.put(engine, new NextProtoNego.ServerProvider() { @Override public void unsupported() { AsyncConnectionFactory connectionFactory = getDefaultAsyncConnectionFactory(); + AsyncEndPoint sslEndPoint = sslEndPointRef.get(); AsyncConnection connection = connectionFactory.newAsyncConnection(channel, sslEndPoint, SPDYServerConnector.this); sslEndPoint.setConnection(connection); } @@ -200,6 +220,7 @@ public class SPDYServerConnector extends SelectChannelConnector public void protocolSelected(String protocol) { AsyncConnectionFactory connectionFactory = getAsyncConnectionFactory(protocol); + AsyncEndPoint sslEndPoint = sslEndPointRef.get(); AsyncConnection connection = connectionFactory.newAsyncConnection(channel, sslEndPoint, SPDYServerConnector.this); sslEndPoint.setConnection(connection); } diff --git a/jetty-spdy/spdy-jetty/src/test/java/org/eclipse/jetty/spdy/SSLEngineLeakTest.java b/jetty-spdy/spdy-jetty/src/test/java/org/eclipse/jetty/spdy/SSLEngineLeakTest.java new file mode 100644 index 00000000000..9605162a914 --- /dev/null +++ b/jetty-spdy/spdy-jetty/src/test/java/org/eclipse/jetty/spdy/SSLEngineLeakTest.java @@ -0,0 +1,48 @@ +package org.eclipse.jetty.spdy; + +import java.lang.reflect.Field; +import java.util.Map; +import java.util.concurrent.Executor; +import java.util.concurrent.TimeUnit; + +import org.eclipse.jetty.npn.NextProtoNego; +import org.eclipse.jetty.spdy.api.Session; +import org.eclipse.jetty.spdy.api.server.ServerSessionFrameListener; +import org.eclipse.jetty.util.ssl.SslContextFactory; +import org.junit.Assert; +import org.junit.Test; + +public class SSLEngineLeakTest extends AbstractTest +{ + @Override + protected SPDYServerConnector newSPDYServerConnector(ServerSessionFrameListener listener) + { + SslContextFactory sslContextFactory = newSslContextFactory(); + return new SPDYServerConnector(listener, sslContextFactory); + } + + @Override + protected SPDYClient.Factory newSPDYClientFactory(Executor threadPool) + { + SslContextFactory sslContextFactory = newSslContextFactory(); + return new SPDYClient.Factory(threadPool, sslContextFactory); + } + + @Test + public void testSSLEngineLeak() throws Exception + { + avoidStackLocalVariables(); + Thread.sleep(1000); + System.gc(); + Field field = NextProtoNego.class.getDeclaredField("objects"); + field.setAccessible(true); + Map objects = (Map)field.get(null); + Assert.assertEquals(0, objects.size()); + } + + private void avoidStackLocalVariables() throws Exception + { + Session session = startClient(startServer(null), null); + session.goAway().get(5, TimeUnit.SECONDS); + } +}