s2a,netty: S2AHandshakerServiceChannel doesn't use custom event loop. #11539
Conversation
| @@ -31,6 +33,36 @@ public final class InternalProtocolNegotiators { | |||
|
|
|||
| private InternalProtocolNegotiators() {} | |||
|
|
|||
There was a problem hiding this comment.
@ejona86, Is it ok to add this new API? I think we need this to access
. Please let me if there's a way to do this without making changes to this file
There was a problem hiding this comment.
Yeah, adding this API is okay. We'd expect s2a to need to dig deep into Netty at times. The benefit of having it in the same repository is we can use internal APIs easily.
rmehta19
force-pushed
the
s2a-handshaker-default-event-loop
branch
from
03ced73
to
a8120bb
Compare
rmehta19
changed the title
| * Returns a {@link ProtocolNegotiator} that ensures the pipeline is set up so that TLS will | ||
| * be negotiated, the {@code handler} is added and writes to the {@link io.netty.channel.Channel} | ||
| * may happen immediately, even before the TLS Handshake is complete. | ||
| * @param executorPool a dedicated {@link Executor} pool for time-consuming TLS tasks |
There was a problem hiding this comment.
You don't utilize or store executorPool
There was a problem hiding this comment.
For some reason, it didn't get included in original commit, fixed now.
| } catch (InterruptedException e) { | ||
| isDelegateTerminated = false; | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
if shutdownNow didn't complete, you'll probably want to know about it for debugging, so I'd suggest logging a warning here.
| @@ -227,7 +228,9 @@ protected void handlerAdded0(ChannelHandlerContext ctx) { | |||
| @Override | |||
| public void onSuccess(SslContext sslContext) { | |||
| ChannelHandler handler = | |||
| InternalProtocolNegotiators.tls(sslContext).newHandler(grpcHandler); | |||
| InternalProtocolNegotiators.tls( | |||
| sslContext, new FixedObjectPool<>(Executors.newFixedThreadPool(1))) | |||
There was a problem hiding this comment.
This creates a new thread, and doesn't shut it down, for each handshake. If we want to do this temporarily (just as a stepping stone), that is fine, but we'll need to do something better here. (So you can worry about this some in a future PR.)
Since this is client-side, it seems fairly safe to just use an (unlimited) cached thread pool across the connections. That could be served by GrpcUtil.SHARED_CHANNEL_EXECUTOR.
There was a problem hiding this comment.
Thanks for the suggestion, it SGTM, I've changed it to use the Shared thread pool: 168d62f
rmehta19
force-pushed
the
s2a-handshaker-default-event-loop
branch
from
ba02591
to
653ac1e
Compare
Addresses #11113 (comment) and #11113 (comment)