Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add remote address information in SessionProtocolNegotiationException #6113

Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ private void connect(SessionProtocol desiredProtocol, SerializationFormat serial
notifyConnect(desiredProtocol, key,
eventLoop.newFailedFuture(
new SessionProtocolNegotiationException(
desiredProtocol, "previously failed negotiation")),
desiredProtocol, remoteAddress, "previously failed negotiation")),
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,
ctx.channel().remoteAddress(), reason)));
ctx.close();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ private void scheduleSessionTimeout(Channel channel, Promise<Channel> sessionPro
sessionTimeoutFuture = channel.eventLoop().schedule(() -> {
if (sessionPromise.tryFailure(new SessionProtocolNegotiationException(
desiredProtocol,
channel.remoteAddress(),
"connection established, but session creation timed out: " + channel))) {
channel.close();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

import static java.util.Objects.requireNonNull;

import java.net.SocketAddress;

import com.linecorp.armeria.common.Flags;
import com.linecorp.armeria.common.SessionProtocol;
import com.linecorp.armeria.common.annotation.Nullable;
Expand All @@ -34,22 +36,30 @@ public final class SessionProtocolNegotiationException extends RuntimeException
private final SessionProtocol actual;

/**
* Creates a new instance with the specified expected {@link SessionProtocol}.
* Creates a new instance with the specified expected {@link SessionProtocol} and {@link SocketAddress}.
*/
public SessionProtocolNegotiationException(SessionProtocol expected, @Nullable String reason) {
super(appendReason("expected: " + requireNonNull(expected, "expected"), reason));
public SessionProtocolNegotiationException(SessionProtocol expected,
@Nullable SocketAddress remoteAddress,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: ...)"

@Nullable String reason) {
super(appendReason("expected: " + requireNonNull(expected, "expected") +
", remoteAddress: " + remoteAddress,
reason));
this.expected = expected;
actual = null;
}

/**
* Creates a new instance with the specified expected and actual {@link SessionProtocol}s.
* Creates a new instance with the specified expected and actual {@link SessionProtocol}s
* and {@link SocketAddress}.
*/
public SessionProtocolNegotiationException(SessionProtocol expected,
@Nullable SessionProtocol actual, @Nullable String reason) {

@Nullable SessionProtocol actual,
@Nullable SocketAddress remoteAddress,
@Nullable String reason) {
super(appendReason("expected: " + requireNonNull(expected, "expected") +
", actual: " + actual, reason));
", actual: " + actual +
", remoteAddress: " + remoteAddress,
reason));
this.expected = expected;
this.actual = actual;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ void shouldRejectH2WithoutAlpn() {
.isInstanceOf(UnprocessedRequestException.class)
.hasCauseInstanceOf(SessionProtocolNegotiationException.class)
.hasMessageContaining("expected: h2, actual: h1, " +
"remoteAddress: 127.0.0.1/127.0.0.1:" + server.httpPort() + ", " +
"reason: unexpected protocol negotiation result");
}
}
Expand Down
Loading