374258 - SPDY leaks SSLEngines

jetty-spdy/spdy-core/src/main/java/org/eclipse/jetty/spdy/Promise.java

@@ -80,7 +80,9 @@ public class Promise<T> implements Handler<T>, Future<T>
     @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();
     }
 

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<AsyncEndPoint> sslEndPointRef = new AtomicReference<>();
+                        final AtomicReference<Object> 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;
                             }
                         });

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<AsyncEndPoint> 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);
                 }

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<Object, NextProtoNego.Provider> objects = (Map<Object, NextProtoNego.Provider>)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);
+    }
+}