Repository: netty/netty
Issue: Let OpenSslContext implement ReferenceCounted?

Hello,

would it make sense to let the `OpenSslContext` class implement the `ReferenceCounted` interface?

We have a dynamic and unbound set of OpenSslContext that are concurrently in use and it would be great if we can release the native resources in a timely manner. We're currently using a wrapper object that extends SslContext, implements ReferenceCounted and gets passed into the SniHandler.

Something along the lines of this pseudo code. I'd be happy to open a PR.

``` java
public class OpenSslContext extends SslContext implements ReferenceCounted {

  private final ReferenceCounted refCnt = new AbstractReferenceCounted() {

    @Override
    public ReferenceCounted touch(Object hint) {
      OpenSslContext.this.hint = hint;
      return this;
    }

    @Override
    protected void deallocate() {
      destroy();
    }
  };

  private Object hint = null;

  @Override
  public int refCnt() {
    return refCnt.refCnt();
  }

  @Override
  public OpenSslContext retain() {
    refCnt.retain();
    return this;
  }

  @Override
  public OpenSslContext retain(int increment) {
    refCnt.retain(increment);
    return this;
  }

  @Override
  public OpenSslContext touch() {
    refCnt.touch();
    return this;
  }

  @Override
  public OpenSslContext touch(Object hint) {
    refCnt.touch(hint);
    return this;
  }

  @Override
  public boolean release() {
    return refCnt.release();
  }

  @Override
  public boolean release(int decrement) {
    return refCnt.release(decrement);
  }

  protected final void destroy() {
    synchronized (OpenSslContext.class) {
      if (ctx != 0) {
        SSLContext.free(ctx);
        ctx = 0;
      }

      // Guard against multiple destroyPools() calls triggered by construction exception and finalize() later
      if (aprPool != 0) {
        Pool.destroy(aprPool);
        aprPool = 0;
      }
    }
  }
}
```


Issue comments:
@rkapsi I think we could do this. That said we would still need a finalizer for the `OpenSslEngine` itself as we never know what the user will do with it (or will use it at all after creating).

@normanmaurer Correct. It's meant to be an optimization for folks who don't want to wait for the GC. The finalizer will still run but potentially do nothing.

@rkapsi are you using the OPENSSL_REFCNT ssl provider in production? I'm currently trying to wrap my head around the usage eg. every call to io.netty.handler.ssl.SslContext#newEngine(io.netty.buffer.ByteBufAllocator) must be matched with releasing it via io.netty.util.ReferenceCountUtil#release(java.lang.Object) or is it enough just to release the io.netty.handler.ssl.SslContext? equally from a client pov it seems it is possible to hack the openssl_refcnt ssl provider into async-http-client using a custom org.asynchttpclient.SslEngineFactory but I suppose each connection may need to be released explicitly too.
@johnou we do. In our case we match `newEngine(...)` to one `release(...)` upon the client's disconnect.

Management of the `SslContext` follows a similar pattern but there is a little twist. We try to re-use an `SslContext` instance for as long as possible (with constraints) by making use of a (heap) caching layer. The flow looks about like this:

A client connects and we go thru the SNI flow. The SNI thingy is backed by the said cache. The cache always increments the refCnt by 1 before returning it and we then do `newEngine(...)`. When the client disconnects we decrement the engine's as well as the context's refCnts by 1. And lastly the cache decrements the context's refCnt upon eviction.

To put it differently, a context's refCnt will always be >= 1 as long as a reference is being held in the cache OR one connection is using it.
@rkapsi perfect, thanks! I am currently investigating implementing it for our server application as we have thousands and thousands of concurrent long lived SSL connections.. at first glance it looks like I don't even need to track the engine as releasing it is already taken care of here io.netty.handler.ssl.SslHandler#handlerRemoved0 :grinning: 
Yea, we do something like this:

```java
class MyHandler extends SslHandler {
  SslContext sslContext;

  void handlerRemoved(...) {
    try {
      super.handlerRemoved(...);
    } finally {
      sslContext.release();
    }
  }
}
```
See also #9626

Pull request review comments:

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java
index 1dceb7885e8..810c22e5bad 100644
--- a/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java
+++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java
@@ -19,7 +19,9 @@
 import io.netty.buffer.ByteBufAllocator;
 import io.netty.buffer.Unpooled;
 import io.netty.handler.codec.base64.Base64;
+import io.netty.util.AbstractReferenceCounted;
 import io.netty.util.CharsetUtil;
+import io.netty.util.ReferenceCounted;
 import io.netty.util.internal.PlatformDependent;
 import io.netty.util.internal.SystemPropertyUtil;
 import io.netty.util.internal.logging.InternalLogger;
@@ -51,7 +53,7 @@
 import static io.netty.handler.ssl.ApplicationProtocolConfig.SelectorFailureBehavior;
 import static io.netty.handler.ssl.ApplicationProtocolConfig.SelectedListenerFailureBehavior;
 
-public abstract class OpenSslContext extends SslContext {
+public abstract class OpenSslContext extends SslContext implements ReferenceCounted {
     private static final byte[] BEGIN_CERT = "-----BEGIN CERTIFICATE-----\n".getBytes(CharsetUtil.US_ASCII);
     private static final byte[] END_CERT = "\n-----END CERTIFICATE-----\n".getBytes(CharsetUtil.US_ASCII);
     private static final byte[] BEGIN_PRIVATE_KEY = "-----BEGIN PRIVATE KEY-----\n".getBytes(CharsetUtil.US_ASCII);
@@ -73,6 +75,28 @@ public abstract class OpenSslContext extends SslContext {
     // TODO: Maybe make configurable ?
     protected static final int VERIFY_DEPTH = 10;
 
+    /**
+     * [4958]: This reference counter provides an optimization that allows the
+     * user to efficiently share a context and also eagerly release its native
+     * resources once it's no longer in use instead of relaying on the Garbage
+     * Collector and object finalization.
+     */
+    private final ReferenceCounted refCnt = new AbstractReferenceCounted() {
+        @SuppressWarnings("unused")
+        private Object hint;
+
+        @Override
+        public ReferenceCounted touch(Object hint) {
+            this.hint = hint;
+            return this;
+        }
+
+        @Override
+        protected void deallocate() {
+            destroy();
+        }
+    };
+
     /** The OpenSSL SSL_CTX object */
     protected volatile long ctx;
     long aprPool;
@@ -387,6 +411,45 @@ protected final void destroy() {
         }
     }
 
+    @Override
+    public int refCnt() {
+        return refCnt.refCnt();
+    }
+
+    @Override
+    public OpenSslContext retain() {
+        refCnt.retain();
+        return this;
+    }
+
+    @Override
+    public OpenSslContext retain(int increment) {
+        refCnt.retain(increment);
+        return this;
+    }
+
+    @Override
+    public OpenSslContext touch() {
+        refCnt.touch();
+        return this;
+    }
+
+    @Override
+    public OpenSslContext touch(Object hint) {
+        refCnt.touch(hint);
+        return this;
+    }
+
+    @Override
+    public boolean release() {
+        return refCnt.release();
+    }
+
+    @Override
+    public boolean release(int decrement) {
+        return refCnt.release(decrement);
+    }
+
     protected static X509Certificate[] certificates(byte[][] chain) {
         X509Certificate[] peerCerts = new X509Certificate[chain.length];
         for (int i = 0; i < peerCerts.length; i++) {


Test patch: