Repository: netty/netty
Issue: useTasks should be set or settable per ReferenceCountedOpenSslContext

### Expected behavior
Each `ReferenceCountedOpenSslContext` instance have its own setting for UseTasks 
### Actual behavior
Currently `USE_TASKS` is a global set from the system property `"io.netty.handler.ssl.openssl.useTasks"` so all `ReferenceCountedOpenSslContext` instances are forced to use that one value.
### Steps to reproduce
No steps needed. Just from reading the code.
### Minimal yet complete reproducer code (or URL to code)
This is where it is set: https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java#L86 

and this is where it is used: https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java#L339
### Netty version
Latest (4.1?)
### JVM version (e.g. `java -version`)
any
### OS version (e.g. `uname -a`)
all those with tcnative/OpenSsl support.

Issue comments:
cc @ejona86 

I see 2 options:
1) provide an explicit setter in `SslContextBuilder` and the value is then plumbed down to where it is used in `ReferenceCountedOpenSslContext`. 
2) `SslContextBuilder` will have some logic to automatically set this value (say if a custom `TrustManagerFactory` was used) and it will be plumbed down similar to (1) above.

For backward compatibility the system property `"io.netty.handler.ssl.openssl.useTasks"` could still be honored.
For some gRPC work, we need to implement a TrustManager that will do blocking RPCs as part of the verification. We believe that means we need `useTasks=true`. But as a library, we aren't in a position to change a global system property. We'd like a way to enable useTasks=true for us, without changing the behavior of other Netty users in the same process.
@ejona86 please note that this will only work for netty-tcnative-boringssl-static for this use-case as only boringssl provides the needed hooks.  

Would it be enough to expose a setter in `ReferenceCountedOpenSslContext` ?
> @ejona86 please note that this will only work for netty-tcnative-boringssl-static for this use-case as only boringssl provides the needed hooks.
> 
> Would it be enough to expose a setter in `ReferenceCountedOpenSslContext` ?

it works at least for me: if the returned value from `SslContextBuilder.build` is a `ReferenceCountedOpenSslContext` then we call this setter for our case.

`ReferenceCountedOpenSslContext` looks like it is returned by the `SslContextBuilder`. Yeah, that would work. A bit strange to mutate the SslContext, but I can see how that would be easy and work. That sounds quite fair. To configure would be an instanceof+cast+set.

Since gRPC allows passing an SslContext during configuration, I think that would work for our internal-to-gRPC use case (which @sanjaypujare is interested in) and another "our users may synchronously reload certificates in the TrustManager implementation" use case.

The boringssl restriction is good to know. What behavior will we see with openssl-backed tcnative? Will it just "not call our TrustManager", or will it be called on an event loop?

Also, @sanjaypujare is willing to send a PR for the change. But I suggested it would be best to discuss the approach first.
> `ReferenceCountedOpenSslContext` looks like it is returned by the `SslContextBuilder`. Yeah, that would work. A bit strange to mutate the SslContext, but I can see how that would be easy and work. That sounds quite fair. To configure would be an instanceof+cast+set.
> 
> Since gRPC allows passing an SslContext during configuration, I think that would work for our internal-to-gRPC use case (which @sanjaypujare is interested in) and another "our users may synchronously reload certificates in the TrustManager implementation" use case.
> 
> The boringssl restriction is good to know. What behavior will we see with openssl-backed tcnative? Will it just "not call our TrustManager", or will it be called on an event loop?

It will call it on the Eventloop
> 
> Also, @sanjaypujare is willing to send a PR for the change. But I suggested it would be best to discuss the approach first.

Yep a PR would be awesome. Let’s do a setter for now. That said we may want to switch the default to true soon


The current `SSLContext.setUseTasks(ctx, USE_TASKS);` call is in the constructor. With the setter approach I will call it again (with possibly a different value) which I assume is okay? Also the setter will allow the caller to set it to false even if "io.netty.handler.ssl.openssl.useTasks" system property is set to true. 
Yep that’s fine

> Am 13.05.2020 um 21:15 schrieb sanjaypujare <notifications@github.com>:
> 
> ﻿
> The current SSLContext.setUseTasks(ctx, USE_TASKS); call is in the constructor. With the setter approach I will call it again (with possibly a different value) which I assume is okay? Also the setter will allow the caller to set it to false even if "io.netty.handler.ssl.openssl.useTasks" system property is set to true.
> 
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or unsubscribe.


Pull request review comments:

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java
index bc72db4c218..27eb43574c0 100644
--- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java
+++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java
@@ -529,8 +529,7 @@ public final void setPrivateKeyMethod(OpenSslPrivateKeyMethod method) {
         }
     }
 
-    // Exposed for testing only
-    final void setUseTasks(boolean useTasks) {
+    public final void setUseTasks(boolean useTasks) {
         Lock writerLock = ctxLock.writeLock();
         writerLock.lock();
         try {


Test patch: