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.

Sign up for GitHub

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

Jump to bottom

netty: return status code unavailable when netty channel has unresolved InetSocketAddress #7023

Merged
merged 3 commits into from May 21, 2020
Merged

netty: return status code unavailable when netty channel has unresolved InetSocketAddress #7023

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
71f56cb
netty: add check for UnresolvedAddressException for netty channels wi…
reggiemcdonald May 8, 2020
52a7b3a
netty: make netty transport test more lightweight
reggiemcdonald May 10, 2020
02ec5f8
netty: change try-catch to try-finally for better clarity
reggiemcdonald May 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java
View file
Open in desktop
Expand Up @@ -103,7 +103,8 @@ public final class NettyChannelBuilder
* Creates a new builder with the given server address. This factory method is primarily intended
* for using Netty Channel types other than SocketChannel. {@link #forAddress(String, int)} should
* generally be preferred over this method, since that API permits delaying DNS lookups and
* noticing changes to DNS.
* noticing changes to DNS. If an unresolved InetSocketAddress is passed in, then it will remain
* unresolved.
*/
@CheckReturnValue
public static NettyChannelBuilder forAddress(SocketAddress serverAddress) {
Expand Down
4 changes: 4 additions & 0 deletions netty/src/main/java/io/grpc/netty/Utils.java
View file
Open in desktop
Expand Up @@ -54,6 +54,7 @@
import java.io.IOException;
import java.lang.reflect.Constructor;
import java.nio.channels.ClosedChannelException;
import java.nio.channels.UnresolvedAddressException;
import java.util.Map;
import java.util.Map.Entry;
import java.util.concurrent.ThreadFactory;
Expand Down Expand Up @@ -270,6 +271,9 @@ public static Status statusFromThrowable(Throwable t) {
if (t instanceof IOException) {
return Status.UNAVAILABLE.withDescription("io exception").withCause(t);
}
if (t instanceof UnresolvedAddressException) {
return Status.UNAVAILABLE.withDescription("unresolved address").withCause(t);
}
if (t instanceof Http2Exception) {
return Status.INTERNAL.withDescription("http2 exception").withCause(t);
}
Expand Down
52 changes: 52 additions & 0 deletions netty/src/test/java/io/grpc/netty/NettyTransportTest.java
View file
Open in desktop
Expand Up @@ -16,16 +16,24 @@

package io.grpc.netty;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

import com.google.common.util.concurrent.SettableFuture;
import io.grpc.ChannelLogger;
import io.grpc.ServerStreamTracer;
import io.grpc.Status;
import io.grpc.internal.AbstractTransportTest;
import io.grpc.internal.ClientTransportFactory;
import io.grpc.internal.FakeClock;
import io.grpc.internal.InternalServer;
import io.grpc.internal.ManagedClientTransport;
import java.net.InetSocketAddress;
import java.nio.channels.UnresolvedAddressException;
import java.util.List;
import java.util.concurrent.TimeUnit;
import org.junit.After;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

Expand Down Expand Up @@ -106,4 +114,48 @@ protected ManagedClientTransport newClientTransport(InternalServer server) {
public void clientChecksInboundMetadataSize_trailer() throws Exception {
// Server-side is flaky due to https://github.com/netty/netty/pull/8332
}

@Test
public void channelHasUnresolvedHostname() throws Exception {
ejona86 marked this conversation as resolved.
Show resolved Hide resolved
server = null;
reggiemcdonald marked this conversation as resolved.
Show resolved Hide resolved
final SettableFuture<Status> future = SettableFuture.create();
ChannelLogger logger = transportLogger();
ManagedClientTransport transport = clientFactory.newClientTransport(
InetSocketAddress.createUnresolved("invalid", 1234),
new ClientTransportFactory.ClientTransportOptions()
.setChannelLogger(logger), logger);
Runnable runnable = transport.start(new ManagedClientTransport.Listener() {
final Throwable failTestException =
new Throwable("transport should have failed and shutdown but didnt");
Copy link
Member

Choose a reason for hiding this comment

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

super-nit: this can just be created within transportReady() itself. I would be fine with merging it like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Override
public void transportShutdown(Status s) {
future.set(s);
}

@Override
public void transportTerminated() {}

@Override
public void transportReady() {
future.setException(failTestException);
}

@Override
public void transportInUse(boolean inUse) {
future.setException(failTestException);
}
});
if (runnable != null) {
runnable.run();
}
try {
Status status = future.get();
assertEquals(Status.Code.UNAVAILABLE, status.getCode());
assertTrue(status.getCause() instanceof UnresolvedAddressException);
Copy link
Member

Choose a reason for hiding this comment

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

You can leave it as-is, but I'll note that assertTrue/assertFalse commonly produce poor error messages if they fail.

This is one of the biggest reasons using Truth can be nice. With Truth, this would be assertThat(status.getCause()).isinstanceof(UnresolvedAddressException.class) (assertThat is typically a static import from the Truth class). Then if it fails, instead of saying "expected true but was false" it can say "expected UnresolvedAddressException but was SomeOtherException" which can greatly aid debugging. It's possible to do that with the JUnit assertion, but you have to create a string and pass it as the first argument. Something to consider for the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that a lot better! Change made

assertEquals("unresolved address", status.getDescription());
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

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

This is cleaner and clearer as a try-finally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

transport.shutdown(Status.UNAVAILABLE.withDescription("test shutdown"));
throw e;
}
}
}
3 changes: 3 additions & 0 deletions netty/src/test/java/io/grpc/netty/UtilsTest.java
View file
Open in desktop
Expand Up @@ -42,6 +42,7 @@
import io.netty.handler.codec.http2.Http2Exception;
import io.netty.handler.codec.http2.Http2Headers;
import io.netty.util.AsciiString;
import java.nio.channels.UnresolvedAddressException;
import java.util.Map;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -61,6 +62,8 @@ public void testStatusFromThrowable() {
Throwable t;
t = new ConnectTimeoutException("msg");
assertStatusEquals(Status.UNAVAILABLE.withCause(t), Utils.statusFromThrowable(t));
t = new UnresolvedAddressException();
assertStatusEquals(Status.UNAVAILABLE.withCause(t), Utils.statusFromThrowable(t));
t = new Http2Exception(Http2Error.INTERNAL_ERROR, "msg");
assertStatusEquals(Status.INTERNAL.withCause(t), Utils.statusFromThrowable(t));
t = new Exception("msg");
Expand Down