Skip to content

Commit

Permalink
Add remote address information in SessionProtocolNegotiationException (
Browse files Browse the repository at this point in the history
…#6113)

Motivation:

Adding the remoteAddress (i.e., the actual host, IP and port) to the exception message will help users understand which specific endpoint was involved in the failed negotiation.
ref: https://discord.com/channels/1087271586832318494/1087272728177942629/1341444142131183718

I appreciate any kind of feedback!

Modifications:

- Added SocketAddress remoteAddress in the exception message of SessionProtocolNegotiationException.

Result:

- Users will see the remoteAddress in the error message when a SessionProtocolNegotiationException occurs.
- No behavioral changes, but the exception message format is slightly modified.

<!--
Visit this URL to learn more about how to write a pull request description:
https://armeria.dev/community/developer-guide#how-to-write-pull-request-description
-->
  • Loading branch information
kth496 authored Feb 25, 2025
1 parent 33219e0 commit 32e7c3e
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,9 @@ private void connect(SessionProtocol desiredProtocol, SerializationFormat serial
notifyConnect(desiredProtocol, key,
eventLoop.newFailedFuture(
new SessionProtocolNegotiationException(
desiredProtocol, "previously failed negotiation")),
desiredProtocol,
"previously failed negotiation (remoteAddress: " +
remoteAddress + ')')),
promise, timingsBuilder);
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,8 @@ void finishWithNegotiationFailure(
final ChannelPipeline pipeline = ctx.pipeline();
pipeline.channel().eventLoop().execute(
() -> pipeline.fireUserEventTriggered(
new SessionProtocolNegotiationException(expected, actual, reason)));
new SessionProtocolNegotiationException(
expected, actual, reason + " (channel: " + pipeline.channel() + ')')));
ctx.close();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ private void scheduleSessionTimeout(Channel channel, Promise<Channel> sessionPro
sessionTimeoutFuture = channel.eventLoop().schedule(() -> {
if (sessionPromise.tryFailure(new SessionProtocolNegotiationException(
desiredProtocol,
"connection established, but session creation timed out: " + channel))) {
"connection established, but session creation timed out. (channel: " + channel + ')'))) {
channel.close();
}
}, connectionTimeoutMillis, TimeUnit.MILLISECONDS);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,16 +93,21 @@ void shouldRejectH2WithoutAlpn() {
.tlsCustomizer(b -> b.trustManager(cert.certificate()))
.build()) {

final BlockingWebClient client = WebClient.builder("h2://127.0.0.1:" + server.httpPort())
final String remote = "127.0.0.1";
final BlockingWebClient client = WebClient.builder("h2://" + remote + ":" + server.httpPort())
.factory(factory)
.build()
.blocking();

assertThatThrownBy(() -> client.get("/"))
.isInstanceOf(UnprocessedRequestException.class)
.hasCauseInstanceOf(SessionProtocolNegotiationException.class)
.hasMessageContaining("expected: h2, actual: h1, " +
"reason: unexpected protocol negotiation result");
.hasMessageContainingAll(
"expected: h2",
"actual: h1",
"reason: unexpected protocol negotiation result",
remote
);
}
}
}

0 comments on commit 32e7c3e

Please sign in to comment.