Add remote address information in SessionProtocolNegotiationException #6113

kth496 · 2025-02-19T12:41:39Z

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.

github-actions · 2025-02-19T13:49:22Z

🔍 Build Scan® (commit: `56ff0c8`)

Job name	Status	Build Scan®
build-ubicloud-standard-16-jdk-8	✅	https://ge.armeria.dev/s/7puztkrd55boa
build-ubicloud-standard-16-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/hvyccxg62kpoy
build-ubicloud-standard-16-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/vi6iwuumm3hvq
build-ubicloud-standard-16-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/e2brg63rgixvs
build-ubicloud-standard-16-jdk-17-leak	✅	https://ge.armeria.dev/s/rooknrtpeg2me
build-ubicloud-standard-16-jdk-11	✅	https://ge.armeria.dev/s/o3vothvu5mj3a
build-macos-latest-jdk-21	✅	https://ge.armeria.dev/s/fyosid64lqnyi

ikhoon · 2025-02-20T04:48:22Z

core/src/main/java/com/linecorp/armeria/client/SessionProtocolNegotiationException.java

-    public SessionProtocolNegotiationException(SessionProtocol expected, @Nullable String reason) {
-        super(appendReason("expected: " + requireNonNull(expected, "expected"), reason));
+    public SessionProtocolNegotiationException(SessionProtocol expected,
+                                               @Nullable SocketAddress remoteAddress,


These are breaking changes. What do you think of adding the remote address to the reason?
If Channel is available when raising SessionProtocolNegotiationException, should we set it as a hint instead? This provides users with more troubleshooting information via Channel.toString().

"...some reason (remoteAddress: ...)" or "...some reason (channel: ...)"

This reverts commit 80c1eed.

…xception" This reverts commit 01b4f4d.

kth496 · 2025-02-20T12:15:41Z

@ikhoon
Thank you for your feedback. I realize I overlooked API compatibility. I have made improvements based on suggestions!

The test modified along with this pull request is failing only on the Windows platform. I don’t have access to a Windows machine, so it’s difficult for me to debug the issue. How would you suggest I approach resolving this?

ikhoon

Thanks! 🙏🙇‍♂️

minwoox

Thanks!

jrhee17 · 2025-02-21T08:10:20Z

core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java

-                                          desiredProtocol, "previously failed negotiation")),
+                                          desiredProtocol,
+                                          "previously failed negotiation (remoteAddress: " +
+                                          remoteAddress + ")")),


Suggested change

remoteAddress + ")")),

remoteAddress + ')')),

jrhee17 · 2025-02-21T08:10:38Z

core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java

@@ -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() + ")")));


Suggested change

expected, actual, reason + " (channel: " + pipeline.channel() + ")")));

expected, actual, reason + " (channel: " + pipeline.channel() + ')')));

jrhee17 · 2025-02-21T08:12:47Z

core/src/main/java/com/linecorp/armeria/client/HttpSessionHandler.java

@@ -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 + ")"))) {


Suggested change

"connection established, but session creation timed out. (channel: " + channel + ")"))) {

"connection established, but session creation timed out. (channel: " + channel + ')'))) {

minwoox · 2025-02-24T12:41:00Z

@kth496 Would you mind fixing the test failure in Window CI?

H2WithoutAlpnTest > shouldRejectH2WithoutAlpn() FAILED
    org.opentest4j.AssertionFailedError: 
    Expecting throwable message:
      "com.linecorp.armeria.client.SessionProtocolNegotiationException: expected: h2, actual: h1, reason: unexpected protocol negotiation result (channel: [id: 0x85e4bff0, L:/127.0.0.1:50963 ! R:/127.0.0.1:50962])"
    to contain:
      ["expected: h2",
        "actual: h1",
        "reason: unexpected protocol negotiation result",
        "R:127.0.0.1"]
    but could not find:
      ["R:127.0.0.1"]

    Throwable that failed the check:

codecov · 2025-02-24T23:13:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.37%. Comparing base (d30a11c) to head (87e93ab).
Report is 7 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #6113       +/-   ##
===========================================
+ Coverage        0   74.37%   +74.37%     
- Complexity      0    22172    +22172     
===========================================
  Files           0     1959     +1959     
  Lines           0    82262    +82262     
  Branches        0    10740    +10740     
===========================================
+ Hits            0    61186    +61186     
- Misses          0    15954    +15954     
- Partials        0     5122     +5122

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…tionexception

minwoox · 2025-02-25T15:13:44Z

Thanks, @kth496! 👍 👍 👍

Add remote address information in SessionProtocolNegotiationException

01b4f4d

kth496 requested review from ikhoon, jrhee17, minwoox and trustin as code owners February 19, 2025 12:41

fix test

80c1eed

ikhoon reviewed Feb 20, 2025

View reviewed changes

ikhoon added the improvement label Feb 20, 2025

kth496 added 5 commits February 20, 2025 19:47

Revert "fix test"

a5451fe

This reverts commit 80c1eed.

Revert "Add remote address information in SessionProtocolNegotiationE…

d11ab43

…xception" This reverts commit 01b4f4d.

Apply review comments

dc28cae

Improve test code

d7aa004

Fix test

e7eedbf

kth496 requested a review from ikhoon February 20, 2025 12:59

ikhoon approved these changes Feb 21, 2025

View reviewed changes

ikhoon added this to the 1.32.0 milestone Feb 21, 2025

minwoox approved these changes Feb 21, 2025

View reviewed changes

jrhee17 approved these changes Feb 21, 2025

View reviewed changes

Apply review comment

accf02c

Fix test

87e93ab

kth496 added 2 commits February 25, 2025 09:00

lint

259f724

Merge branch 'main' into add-remote-address-to-sessionprotocolnegotia…

56ff0c8

…tionexception

minwoox merged commit 32e7c3e into line:main Feb 25, 2025
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add remote address information in SessionProtocolNegotiationException #6113

Add remote address information in SessionProtocolNegotiationException #6113

kth496 commented Feb 19, 2025

github-actions bot commented Feb 19, 2025 •

edited

Loading

ikhoon Feb 20, 2025

kth496 commented Feb 20, 2025

ikhoon left a comment

minwoox left a comment

jrhee17 Feb 21, 2025

jrhee17 Feb 21, 2025

jrhee17 Feb 21, 2025

minwoox commented Feb 24, 2025

codecov bot commented Feb 24, 2025

minwoox commented Feb 25, 2025

	expected, actual, reason + " (channel: " + pipeline.channel() + ")")));
	expected, actual, reason + " (channel: " + pipeline.channel() + ')')));

	"connection established, but session creation timed out. (channel: " + channel + ")"))) {
	"connection established, but session creation timed out. (channel: " + channel + ')'))) {

Add remote address information in SessionProtocolNegotiationException #6113

Add remote address information in SessionProtocolNegotiationException #6113

Conversation

kth496 commented Feb 19, 2025

github-actions bot commented Feb 19, 2025 • edited Loading

🔍 Build Scan® (commit: 56ff0c8)

ikhoon Feb 20, 2025

Choose a reason for hiding this comment

kth496 commented Feb 20, 2025

ikhoon left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 Feb 21, 2025

Choose a reason for hiding this comment

jrhee17 Feb 21, 2025

Choose a reason for hiding this comment

jrhee17 Feb 21, 2025

Choose a reason for hiding this comment

minwoox commented Feb 24, 2025

codecov bot commented Feb 24, 2025

Codecov Report

minwoox commented Feb 25, 2025

github-actions bot commented Feb 19, 2025 •

edited

Loading

🔍 Build Scan® (commit: `56ff0c8`)