netty: return status code unavailable when netty channel has unresolved InetSocketAddress #7023
Conversation
…th unresolved InetSocketAddresses This commit adds a check for UnresolvedAddressException in netty Utils. This change was made to ensure that netty channels built from an unresolved InetSocketAddress would return Status UNAVAILABLE instead of UNKNOWN.
| public void channelHasUnresolvedHostname() throws Exception { | ||
| server = null; | ||
| ManagedChannel channel = NettyChannelBuilder | ||
| .forAddress(new InetSocketAddress("invalid", 1234)) |
There was a problem hiding this comment.
InetSocketAddress.createUnresolved()
There was a problem hiding this comment.
ahh missed this in the docs. Thanks!
This commit addresses PR feedback to make NettyTransportTest.channelHasUnresolvedHost more lightweight.
There was a problem hiding this comment.
Only thing that actually matters is the try-finally, and I can even live with it. I'll give you some time to fix it, but if it looks like it'll take more than a day or two I'd be happy to merge as-is. (I'm not saying that to rush you, just to say how it doesn't matter much.)
| .setChannelLogger(logger), logger); | ||
| Runnable runnable = transport.start(new ManagedClientTransport.Listener() { | ||
| final Throwable failTestException = | ||
| new Throwable("transport should have failed and shutdown but didnt"); |
There was a problem hiding this comment.
super-nit: this can just be created within transportReady() itself. I would be fine with merging it like this.
| assertEquals(Status.Code.UNAVAILABLE, status.getCode()); | ||
| assertTrue(status.getCause() instanceof UnresolvedAddressException); | ||
| assertEquals("unresolved address", status.getDescription()); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
This is cleaner and clearer as a try-finally.
| try { | ||
| Status status = future.get(); | ||
| assertEquals(Status.Code.UNAVAILABLE, status.getCode()); | ||
| assertTrue(status.getCause() instanceof UnresolvedAddressException); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I like that a lot better! Change made
This commit addresses a PR comment to make the try-catch a try-finally. Swaps assertTrue to Truth.assertThat.
|
Just wanted to check whether this PR needs any additional changes, or if its ready to merge? |
|
oops sorry about the delay. I blame kokoro |
|
Yep. I needed kokoro to run and then it got buried. Thanks for pinging it! |
|
No problem! 🙂 |
…ed InetSocketAddress (grpc#7023)
Closes #6954
Bug fix to return status code
UNAVAILABLEwhen a netty channel has an unresolved InetSocketAddressUNAVAILABLEis returned