Skip to content

Commit

Permalink
netty: Eagerly create SslContext
Browse files Browse the repository at this point in the history
Creating the SslContext can throw, generally due to broken ALPN. We want
that to propagate to the caller of build(), instead of within the
channel where it could easily cause hangs.

We still delay creation until actual build() time, since TLS is not
guaranteed to work and the application may be configuring plaintext or
similar later before calling build() where SslContext is unnecessary.

The only externally-visible change should be the exception handling.
I'd add a test, but the things throwing are static and trying to inject
them would be pretty messy.

Fixes #2599
  • Loading branch information
ejona86 committed Jun 6, 2017
1 parent e4f1f39 commit 4d740a5
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 16 deletions.
14 changes: 7 additions & 7 deletions netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -394,13 +394,6 @@ private static ProtocolNegotiator createProtocolNegotiatorByType(
case PLAINTEXT_UPGRADE:
return ProtocolNegotiators.plaintextUpgrade();
case TLS:
if (sslContext == null) {
try {
sslContext = GrpcSslContexts.forClient().build();
} catch (SSLException ex) {
throw new RuntimeException(ex);
}
}
return ProtocolNegotiators.tls(sslContext, authority);
default:
throw new IllegalArgumentException("Unsupported negotiationType: " + negotiationType);
Expand Down Expand Up @@ -472,6 +465,13 @@ private static final class NettyTransportFactory implements ClientTransportFacto
this.channelType = channelType;
this.negotiationType = negotiationType;
this.channelOptions = new HashMap<ChannelOption<?>, Object>(channelOptions);
if (negotiationType == NegotiationType.TLS && sslContext == null) {
try {
sslContext = GrpcSslContexts.forClient().build();
} catch (SSLException ex) {
throw new RuntimeException(ex);
}
}
this.sslContext = sslContext;

if (transportCreationParamsFilterFactory == null) {
Expand Down
13 changes: 4 additions & 9 deletions netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -158,16 +158,11 @@ public void createProtocolNegotiator_plaintextUpgrade() {

@Test
public void createProtocolNegotiator_tlsWithNoContext() {
ProtocolNegotiator negotiator = NettyChannelBuilder.createProtocolNegotiator(
thrown.expect(NullPointerException.class);
NettyChannelBuilder.createProtocolNegotiator(
"authority:1234",
NegotiationType.TLS,
noSslContext);

assertTrue(negotiator instanceof ProtocolNegotiators.TlsNegotiator);
ProtocolNegotiators.TlsNegotiator n = (TlsNegotiator) negotiator;

assertEquals("authority", n.getHost());
assertEquals(1234, n.getPort());
}

@Test
Expand All @@ -185,11 +180,11 @@ public void createProtocolNegotiator_tlsWithClientContext() throws SSLException
}

@Test
public void createProtocolNegotiator_tlsWithAuthorityFallback() {
public void createProtocolNegotiator_tlsWithAuthorityFallback() throws SSLException {
ProtocolNegotiator negotiator = NettyChannelBuilder.createProtocolNegotiator(
"bad_authority",
NegotiationType.TLS,
noSslContext);
GrpcSslContexts.forClient().build());

assertTrue(negotiator instanceof ProtocolNegotiators.TlsNegotiator);
ProtocolNegotiators.TlsNegotiator n = (TlsNegotiator) negotiator;
Expand Down

0 comments on commit 4d740a5

Please sign in to comment.