SslHandler handlerRemoved0 method doesn't always release the SSLEngine

[hunter2046]

Expected behavior

When ReferenceCountedOpenSslEngine is used the SslHandler's handlerRemoved0 method should release the SSLEngine

Actual behavior

When ReferenceCountedOpenSslEngine is used the SslHandler's handlerRemoved0 method doesn't release the SSLEngine if it fails in the middle

Steps to reproduce

• made some modifications to the handlerRemoved0 for more logging and catching potential throwables:

@Override
  public void handlerRemoved0(ChannelHandlerContext ctx) throws Exception {
    final ReferenceCountedOpenSslEngine sslEngine = (ReferenceCountedOpenSslEngine) engine;
    logger.info("====SslHandler handlerRemoved0 Step 1, sslEngine: {}, refCnt(): {}, channel id: {}", sslEngine, sslEngine.refCnt(), ctx.channel());

    if (!pendingUnencryptedWrites.isEmpty()) {
      // Check if queue is not empty first because create a new ChannelException is expensive
      pendingUnencryptedWrites.releaseAndFailAll(ctx,
        new ChannelException("Pending write on removal of SslHandler"));
    }
    pendingUnencryptedWrites = null;

    SSLHandshakeException cause = null;

    try {
      logger.info("====SslHandler handlerRemoved0 Step 2, sslEngine: {}, refCnt(): {}, channel id: {}", sslEngine, sslEngine.refCnt(), ctx.channel());

      // If the handshake is not done yet we should fail the handshake promise and notify the rest of the
      // pipeline.
      if (!handshakePromise.isDone()) {
        cause = new SSLHandshakeException("SslHandler removed before handshake completed");
        if (handshakePromise.tryFailure(cause)) {
          ctx.fireUserEventTriggered(new SslHandshakeCompletionEvent(cause));
        }
      }

      logger.info("====SslHandler handlerRemoved0 Step 3, sslEngine: {}, refCnt(): {}, channel id: {}", sslEngine, sslEngine.refCnt(), ctx.channel());

      if (!sslClosePromise.isDone()) {
        if (cause == null) {
          cause = new SSLHandshakeException("SslHandler removed before handshake completed");
        }
        notifyClosePromise(cause);
      }
    } catch (Throwable e) {
      logger.info("====SslHandler handlerRemoved0 exception, sslEngine: {}, refCnt(): {}, channel id: {}, e: {}, e.getMessage(): {}", sslEngine, sslEngine.refCnt(), ctx.channel(), e, e.getMessage());
      throw e;
    }

    logger.info("====SslHandler handlerRemoved0 Step 4, sslEngine: {}, refCnt(): {}, channel id: {}", sslEngine, sslEngine.refCnt(), ctx.channel());

    if (engine instanceof ReferenceCounted) {
      ((ReferenceCounted) engine).release();
    }

    logger.info("====SslHandler handlerRemoved0 Step 5, sslEngine: {}, refCnt(): {}, channel id: {}", sslEngine, sslEngine.refCnt(), ctx.channel());

  }

• allocated a relatively small heap size to run some code which repeatedly creates SslHandler instances with the use of ReferenceCountedOpenSslEngine
• observed below logs

 2021-08-18 11:04:38,586 (client-io-3-2) [INFO - io.netty.handler.ssl.SslHandler.handlerRemoved0(SslHandler.java:691)]              ====SslHandler handlerRemoved0 Step 2, sslEngine: io.netty.handler.ssl.ReferenceCountedOpenSslEngine@6027ec2, refCnt(): 1, channel     id: [id: 0x462cc9e4]
 2021-08-18 11:04:39,362 (client-io-3-2) [INFO - io.netty.handler.ssl.SslHandler.handlerRemoved0(SslHandler.java:712)]              ====SslHandler handlerRemoved0 exception, sslEngine: io.netty.handler.ssl.ReferenceCountedOpenSslEngine@6027ec2, refCnt(): 1, channel  id: [id: 0x462cc9e4], e: java.lang.OutOfMemoryError: GC overhead limit exceeded, e.getMessage(): GC overhead limit exceeded

This indicates that the sslEngine did not get released. Does it make sense to put it in a final block to ensure it is released?

Minimal yet complete reproducer code (or URL to code)

I don't have a running reproducer. The above code should describe the problem clearly though.

Netty version

4.1.67.Final

JVM version (e.g. java -version)

java version "1.8.0_271"
Java(TM) SE Runtime Environment (build 1.8.0_271-b09)
Java HotSpot(TM) 64-Bit Server VM (build 25.271-b09, mixed mode)

OS version (e.g. uname -a)

Darwin cliu-MBP-CBD58 19.6.0 Darwin Kernel Version 19.6.0: Thu May  6 00:48:39 PDT 2021; root:xnu-6153.141.33~1/RELEASE_X86_64 x86_64

[NiteshKant]

Putting the release in a try-finally block does make sense to me. Do you want to send a PR?

[hunter2046]

Thanks for checking. I will find some time this week to get a PR out.

[hunter2046]

PR has been merged - closing this issue