Let OpenSslContext implement ReferenceCounted?

[rkapsi]

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.

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

[normanmaurer]

@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).

[rkapsi]

@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.

[johnou]

@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.

[rkapsi]

@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.

[johnou]

@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 😀

[rkapsi]

Yea, we do something like this:

class MyHandler extends SslHandler {
  SslContext sslContext;

  void handlerRemoved(...) {
    try {
      super.handlerRemoved(...);
    } finally {
      sslContext.release();
    }
  }
}

[njhill]

See also #9626