Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for OpenSslEngine with no finalizer #1669

Merged
merged 1 commit into from
Oct 16, 2019

Conversation

johnou
Copy link
Contributor

@johnou johnou commented Oct 15, 2019

Motivation:

Support for Netty SslProvider.OPENSSL_REFCNT (OpenSSL-based implementation which does not have finalizers and instead implements ReferenceCounted).

Modification:

Add destroy method to SslEngineFactory to allow cleaning up reference counted SslContext.

Result:

Users can opt-in to a finalizer free OpenSslEngine and OpenSslContext.

@johnou johnou force-pushed the support_openssl_refcnt branch from bba18cf to 2d3185a Compare October 15, 2019 21:57
@johnou
Copy link
Contributor Author

johnou commented Oct 15, 2019

Currently enabling openssl_refcnt using a custom DefaultSslEngineFactory..

public static class ReferenceCountedOpenSslEngineFactory extends DefaultSslEngineFactory {

  private SslProvider provider;

  @Override
  public void init(AsyncHttpClientConfig config) throws SSLException {
    this.provider = config.isUseOpenSsl() ? SslProvider.OPENSSL_REFCNT : SslProvider.JDK;
    super.init(config);
  }

  @Override
  protected SslContextBuilder configureSslContextBuilder(SslContextBuilder builder) {
    return builder.sslProvider(provider);
  }
}

Motivation:

Support for Netty SslProvider.OPENSSL_REFCNT (OpenSSL-based implementation which does not have finalizers and instead implements ReferenceCounted).

Modification:

Add destroy method to SslEngineFactory to allow cleaning up reference counted SslContext.

Result:

Users can opt-in to a finalizer free OpenSslEngine and OpenSslContext.
@johnou johnou force-pushed the support_openssl_refcnt branch from 2d3185a to e6adb5e Compare October 15, 2019 22:19
@johnou
Copy link
Contributor Author

johnou commented Oct 16, 2019

As a side note I ran the ATC testsuite with -Dio.netty.leakDetectionLevel=paranoid which passed without any leak reports.

@slandelle slandelle merged commit a7ea7cf into AsyncHttpClient:master Oct 16, 2019
@slandelle
Copy link
Contributor

Thanks!

@slandelle
Copy link
Contributor

As a side issue, it would be nice to be able to enable from config instead of having to go with a custom SslEngineFactory.

@johnou
Copy link
Contributor Author

johnou commented Oct 16, 2019

@slandelle I did consider that, however, OPENSSL_REFCNT is currently marked UnstableApi and wanted to avoid dependency issues with Netty.

@johnou johnou deleted the support_openssl_refcnt branch October 16, 2019 09:00
@slandelle
Copy link
Contributor

slandelle commented Oct 16, 2019

Do you mean you're not sure OPENSSL_REFCNT is production ready yet and you want to experiment first?

@johnou
Copy link
Contributor Author

johnou commented Oct 16, 2019

I believe it is production ready, however it is still marked "unstable" in Netty here https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/SslProvider.java#L37

@slandelle
Copy link
Contributor

OK, let's leave as is for now.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants