How this was discovered
I was diagnosing an integration test that was expected to fail (the SNI
hostname was not mapped) but kept passing instead. Tracing through, the
lookup callback was throwing on the unmapped hostname, but the exception
never reached exceptionCaught(...) — SslClientHelloHandler.decode()
caught it in its outer catch (Exception) block and retried via
select(ctx, null). The default fallback via select(ctx, null) allowed the handshake
to complete; the test passed when it should have failed.
What I observed
When lookup(...) returns an already-completed Future (sync path) and
onLookupComplete(...) throws an exception:
select(ctx, null) is invoked as a retry, allowing the handshake to fall through to the default SslContext path.
exceptionCaught(...) is never invoked on the pipeline.
When the same callback is exercised on the async path (an unresolved
Future that is completed later), the exception is correctly routed via
ctx.fireExceptionCaught(...) and no select(ctx, null) retry occurs.
So the same user callback, with the same exception, produces opposite
outcomes depending on whether the user's Promise is completed synchronously
or asynchronously.
Where in the code
handler/src/main/java/io/netty/handler/ssl/SslClientHelloHandler.java:
// decode(), ~L196-202 — outer catch in decode()
} catch (Exception e) {
if (logger.isDebugEnabled()) {
logger.debug("Unexpected client hello packet: " + ByteBufUtil.hexDump(in), e);
}
select(ctx, null);
}
// select(), ~L217-255
private void select(final ChannelHandlerContext ctx, ByteBuf clientHello) throws Exception {
final Future<T> future;
try {
future = lookup(ctx, clientHello);
if (future.isDone()) {
onLookupComplete(ctx, future); // ← sync path: no try/catch
} else {
future.addListener((FutureListener<T>) future1 -> {
releaseIfNotNull(finalClientHello);
try {
suppressRead = false;
try {
onLookupComplete(ctx, future1);
} catch (DecoderException err) {
ctx.fireExceptionCaught(err); // ← async path: routes to pipeline
} catch (Exception cause) {
ctx.fireExceptionCaught(new DecoderException(cause));
} catch (Throwable cause) {
ctx.fireExceptionCaught(cause);
}
} finally { /* readPending */ }
});
}
} catch (Throwable cause) {
PlatformDependent.throwException(cause);
} finally {
releaseIfNotNull(clientHello);
}
}
On the sync branch, the exception thrown by onLookupComplete propagates up
through select()'s outer catch (Throwable) (re-thrown via
PlatformDependent.throwException) and ends up caught by decode()'s
catch (Exception e), which then calls select(ctx, null).
Suggested fix
Apply the same inner routing to the sync branch that the async branch already
has:
try {
future = lookup(ctx, clientHello);
if (future.isDone()) {
- onLookupComplete(ctx, future);
+ try {
+ onLookupComplete(ctx, future);
+ } catch (DecoderException err) {
+ ctx.fireExceptionCaught(err);
+ } catch (Exception cause) {
+ ctx.fireExceptionCaught(new DecoderException(cause));
+ } catch (Throwable cause) {
+ ctx.fireExceptionCaught(cause);
+ }
} else {
suppressRead = true;
With this change the sync path no longer performs the select(ctx, null) retry on user-callback exceptions; instead, the exception is routed via exceptionCaught(...) like on the async path.
I'm willing to submit a PR if this is accepted.
How this was discovered
I was diagnosing an integration test that was expected to fail (the SNI
hostname was not mapped) but kept passing instead. Tracing through, the
lookup callback was throwing on the unmapped hostname, but the exception
never reached
exceptionCaught(...)—SslClientHelloHandler.decode()caught it in its outer
catch (Exception)block and retried viaselect(ctx, null). The default fallback via select(ctx, null) allowed the handshaketo complete; the test passed when it should have failed.
What I observed
When
lookup(...)returns an already-completedFuture(sync path) andonLookupComplete(...)throws an exception:select(ctx, null)is invoked as a retry, allowing the handshake to fall through to the default SslContext path.exceptionCaught(...)is never invoked on the pipeline.When the same callback is exercised on the async path (an unresolved
Futurethat is completed later), the exception is correctly routed viactx.fireExceptionCaught(...)and noselect(ctx, null)retry occurs.So the same user callback, with the same exception, produces opposite
outcomes depending on whether the user's
Promiseis completed synchronouslyor asynchronously.
Where in the code
handler/src/main/java/io/netty/handler/ssl/SslClientHelloHandler.java:On the sync branch, the exception thrown by
onLookupCompletepropagates upthrough
select()'s outercatch (Throwable)(re-thrown viaPlatformDependent.throwException) and ends up caught bydecode()'scatch (Exception e), which then callsselect(ctx, null).Suggested fix
Apply the same inner routing to the sync branch that the async branch already
has:
try { future = lookup(ctx, clientHello); if (future.isDone()) { - onLookupComplete(ctx, future); + try { + onLookupComplete(ctx, future); + } catch (DecoderException err) { + ctx.fireExceptionCaught(err); + } catch (Exception cause) { + ctx.fireExceptionCaught(new DecoderException(cause)); + } catch (Throwable cause) { + ctx.fireExceptionCaught(cause); + } } else { suppressRead = true;With this change the sync path no longer performs the select(ctx, null) retry on user-callback exceptions; instead, the exception is routed via exceptionCaught(...) like on the async path.
I'm willing to submit a PR if this is accepted.