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

Close delegate resolver from RoundRobinInetAddressResolver #9214

Merged

Conversation

Projects
None yet
3 participants
@jchambers
Copy link
Contributor

commented Jun 2, 2019

Motivation:

RoundRobinDnsAddressResolverGroup ultimately opens UDP ports for DNS resolution. Callers likely expect that RoundRobinDnsAddressResolverGroup#close() will close those ports, but that is not currently true (see #9212).

Modifications:

Overrode RoundRobinInetAddressResolver#close() to close the delegate name resolver, which in turn closes any UDP ports used for name resolution.

Result:

RoundRobinDnsAddressResolverGroup#close() closes UDP ports as expected. This fixes #9212.

@netty-bot

This comment has been minimized.

Copy link

commented Jun 2, 2019

Can one of the admins verify this patch?

@jchambers jchambers force-pushed the jchambers:close_round_robin_inet_address_resolver branch from f9acc02 to 78bf05f Jun 2, 2019

@jchambers

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

I'm very, very open to feedback on the test for this case. I had to bend a lot of stuff to make the Channel in question visible to the test, and none of the options felt really great to me.

@jchambers

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

I threw in 12616f0 as a suggestion to relieve some (but not all) of the visibility tension in the test. I realized that even though RoundRobinInetAddressResolver isn't specifically about DNS in theory, it's used exclusively by RoundRobinDnsAddressResolverGroup in practice. With that in mind, I moved it to the resolver-dns module and made it package-private.

I can think of lots of reasons this is perhaps not the best idea (chief among them: folks may already be depending directly on RoundRobinInetAddressResolver in the wild), but wanted to throw it out as a suggestion all the same. I'm happy to drop it or move it to its own pull request if it seems controversial.

@@ -36,8 +38,8 @@
* Use {@link #asAddressResolver()} to create a {@link InetSocketAddress} resolver
*/
@UnstableApi
public class RoundRobinInetAddressResolver extends InetNameResolver {
private final NameResolver<InetAddress> nameResolver;
class RoundRobinInetAddressResolver extends InetNameResolver {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 3, 2019

Member

Please don't move it if you can fix the problem in another way as it will "break" people if they depend on the class in their code.

This comment has been minimized.

Copy link
@jchambers

jchambers Jun 3, 2019

Author Contributor

No problem! Rolled it back.

@jchambers jchambers force-pushed the jchambers:close_round_robin_inet_address_resolver branch from 12616f0 to 78bf05f Jun 3, 2019

@@ -91,6 +91,10 @@ public void operationComplete(Future<List<InetAddress>> future) throws Exception
});
}

public NameResolver<InetAddress> nameResolver() {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 3, 2019

Member

can we keep this package-private for now ?

This comment has been minimized.

Copy link
@jchambers

jchambers Jun 3, 2019

Author Contributor

Let me give it a second look. Because RoundRobinInetAddressResolver RoundRobinDnsAddressResolverGroup lives in resolver-dns and a lot of these classes just live in resolver, I think something has to be more public than we'd like, but I could be mistaken.

This comment has been minimized.

Copy link
@jchambers

jchambers Jun 3, 2019

Author Contributor

As a compromise, we could skip testing RoundRobinDnsAddressResolverGroup and test RoundRobinInetAddressResolver directly instead. We wouldn't quite replicate the most common use case, but that would allow us to keep pretty much everything package-private.

This comment has been minimized.

Copy link
@jchambers

jchambers Jun 3, 2019

Author Contributor

I lied.

Moving the test to the resolver module means that we lose visibility on InflightNameResolver. That does have a FIXME, though:

// FIXME(trustin): Find a better name and move it to the 'resolver' module.

We could try to resolve that as part of this pull request, but that feels like a lot of added scope to me.

So to recap, I think our options are:

  1. Do what we have here and publicize some stuff we'd rather not publicize.
  2. Narrow the test to cover RoundRobinInetAddressResolver instead of RoundRobinDnsAddressResolverGroup and resolve the FIXME in InflightNameResolver.
  3. Forget about the test and just ship the simple fix if we think the other two options are too costly.

What do you think?

@@ -100,4 +100,13 @@ public void operationComplete(Future<List<InetAddress>> future) throws Exception
private static int randomIndex(int numAddresses) {
return numAddresses == 1 ? 0 : PlatformDependent.threadLocalRandom().nextInt(numAddresses);
}

public NameResolver<InetAddress> nameResolver() {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 3, 2019

Member

can we keep this package-private for now ?

assertTrue(dnsNameResolver.ch.isOpen());

roundRobinGroup.close();
dnsNameResolver.ch.closeFuture().await(2, TimeUnit.SECONDS);

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 3, 2019

Member

Should we just use sync() and use @Test(timeout = 2000) ?

This comment has been minimized.

Copy link
@jchambers

jchambers Jun 3, 2019

Author Contributor

Sure!

This comment has been minimized.

Copy link
@jchambers

jchambers Jun 3, 2019

Author Contributor

Done and pushed.

@jchambers jchambers force-pushed the jchambers:close_round_robin_inet_address_resolver branch from 78bf05f to 796177f Jun 3, 2019

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Close delegate resolver from RoundRobinInetAddressResolver
Motivation:

RoundRobinDnsAddressResolverGroup ultimately opens UDP
ports for DNS resolution. Callers likely expect that
RoundRobinDnsAddressResolverGroup#close() will close those
ports, but that is not currently true (see #9212).

Modifications:

Overrode RoundRobinInetAddressResolver#close() to close
the delegate name resolver, which in turn closes any UDP
ports used for name resolution.

Result:

RoundRobinDnsAddressResolverGroup#close() closes UDP ports
as expected. This fixes #9212.

@jchambers jchambers force-pushed the jchambers:close_round_robin_inet_address_resolver branch from 796177f to 97d297b Jun 3, 2019

@jchambers

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Stripped it down to the basics. Think we're good to go.

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

@netty-bot test this please

@normanmaurer normanmaurer merged commit f194aed into netty:4.1 Jun 4, 2019

3 checks passed

pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@jchambers thanks a lot!

@normanmaurer normanmaurer added this to the 4.1.37.Final milestone Jun 4, 2019

@normanmaurer normanmaurer self-assigned this Jun 4, 2019

@normanmaurer normanmaurer added the defect label Jun 4, 2019

normanmaurer added a commit that referenced this pull request Jun 4, 2019

Close delegate resolver from RoundRobinInetAddressResolver (#9214)
Motivation:

RoundRobinDnsAddressResolverGroup ultimately opens UDP
ports for DNS resolution. Callers likely expect that
RoundRobinDnsAddressResolverGroup#close() will close those
ports, but that is not currently true (see #9212).

Modifications:

Overrode RoundRobinInetAddressResolver#close() to close
the delegate name resolver, which in turn closes any UDP
ports used for name resolution.

Result:

RoundRobinDnsAddressResolverGroup#close() closes UDP ports
as expected. This fixes #9212.
@jchambers

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

🤘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.