From 6e7c0298de14c1af2ad5aa4161f6bde054f6bb74 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Sun, 7 Jun 2020 00:50:15 -0400 Subject: [PATCH] Remove Netty async DNS resolver completely It "mostly works" - but it's not good enough. Instead, we'll offload the DNS resolution outside the event loop. This is a middle-ground approach between doing the resolution on the calling method (and potentially a Netty I/O thread) and using the intermittently broken Netty async DNS resolver. --- .../proxy/network/ConnectionManager.java | 20 +++-- ...dressResolverGroupNameResolverAdapter.java | 68 ---------------- .../netty/SeparatePoolInetNameResolver.java | 79 +++++++++++++++++++ 3 files changed, 88 insertions(+), 79 deletions(-) delete mode 100644 proxy/src/main/java/com/velocitypowered/proxy/network/netty/DnsAddressResolverGroupNameResolverAdapter.java create mode 100644 proxy/src/main/java/com/velocitypowered/proxy/network/netty/SeparatePoolInetNameResolver.java diff --git a/proxy/src/main/java/com/velocitypowered/proxy/network/ConnectionManager.java b/proxy/src/main/java/com/velocitypowered/proxy/network/ConnectionManager.java index 0f89109df..24fe82440 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/network/ConnectionManager.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/network/ConnectionManager.java @@ -6,7 +6,7 @@ import static org.asynchttpclient.Dsl.config; import com.google.common.base.Preconditions; import com.velocitypowered.natives.util.Natives; import com.velocitypowered.proxy.VelocityServer; -import com.velocitypowered.proxy.network.netty.DnsAddressResolverGroupNameResolverAdapter; +import com.velocitypowered.proxy.network.netty.SeparatePoolInetNameResolver; import com.velocitypowered.proxy.protocol.netty.GS4QueryHandler; import io.netty.bootstrap.Bootstrap; import io.netty.bootstrap.ServerBootstrap; @@ -18,6 +18,7 @@ import io.netty.channel.WriteBufferWaterMark; import io.netty.channel.epoll.EpollChannelOption; import io.netty.resolver.dns.DnsAddressResolverGroup; import io.netty.resolver.dns.DnsNameResolverBuilder; +import io.netty.util.concurrent.GlobalEventExecutor; import java.net.InetSocketAddress; import java.util.HashMap; import java.util.Map; @@ -48,7 +49,7 @@ public final class ConnectionManager { @SuppressWarnings("WeakerAccess") public final BackendChannelInitializerHolder backendChannelInitializer; - private final DnsAddressResolverGroup resolverGroup; + private final SeparatePoolInetNameResolver resolver; private final AsyncHttpClient httpClient; /** @@ -65,21 +66,16 @@ public final class ConnectionManager { new ServerChannelInitializer(this.server)); this.backendChannelInitializer = new BackendChannelInitializerHolder( new BackendChannelInitializer(this.server)); - this.resolverGroup = new DnsAddressResolverGroup(new DnsNameResolverBuilder() - .channelType(this.transportType.datagramChannelClass) - .negativeTtl(15) - .ndots(1)); + this.resolver = new SeparatePoolInetNameResolver(GlobalEventExecutor.INSTANCE); this.httpClient = asyncHttpClient(config() .setEventLoopGroup(this.workerGroup) .setUserAgent(server.getVersion().getName() + "/" + server.getVersion().getVersion()) .addRequestFilter(new RequestFilter() { @Override - public FilterContext filter(FilterContext ctx) throws FilterException { + public FilterContext filter(FilterContext ctx) { return new FilterContextBuilder<>(ctx) .request(new RequestBuilder(ctx.getRequest()) - .setNameResolver( - new DnsAddressResolverGroupNameResolverAdapter(resolverGroup, workerGroup) - ) + .setNameResolver(resolver) .build()) .build(); } @@ -162,7 +158,7 @@ public final class ConnectionManager { .option(ChannelOption.CONNECT_TIMEOUT_MILLIS, this.server.getConfiguration().getConnectTimeout()) .group(group == null ? this.workerGroup : group) - .resolver(this.resolverGroup); + .resolver(this.resolver.asGroup()); if (transportType == TransportType.EPOLL && server.getConfiguration().useTcpFastOpen()) { bootstrap.option(EpollChannelOption.TCP_FASTOPEN_CONNECT, true); } @@ -194,6 +190,8 @@ public final class ConnectionManager { Thread.currentThread().interrupt(); } } + + this.resolver.shutdown(); } public EventLoopGroup getBossGroup() { diff --git a/proxy/src/main/java/com/velocitypowered/proxy/network/netty/DnsAddressResolverGroupNameResolverAdapter.java b/proxy/src/main/java/com/velocitypowered/proxy/network/netty/DnsAddressResolverGroupNameResolverAdapter.java deleted file mode 100644 index 169b60a49..000000000 --- a/proxy/src/main/java/com/velocitypowered/proxy/network/netty/DnsAddressResolverGroupNameResolverAdapter.java +++ /dev/null @@ -1,68 +0,0 @@ -package com.velocitypowered.proxy.network.netty; - -import io.netty.channel.EventLoopGroup; -import io.netty.resolver.InetNameResolver; -import io.netty.resolver.dns.DnsAddressResolverGroup; -import io.netty.util.concurrent.EventExecutor; -import io.netty.util.concurrent.FutureListener; -import io.netty.util.concurrent.ImmediateEventExecutor; -import io.netty.util.concurrent.Promise; -import io.netty.util.internal.ThreadExecutorMap; -import java.net.InetAddress; -import java.net.InetSocketAddress; -import java.util.ArrayList; -import java.util.List; - -public class DnsAddressResolverGroupNameResolverAdapter extends InetNameResolver { - - private final DnsAddressResolverGroup resolverGroup; - private final EventLoopGroup group; - - /** - * Creates a DnsAddressResolverGroupNameResolverAdapter. - * @param resolverGroup the resolver group to use - * @param group the event loop group - */ - public DnsAddressResolverGroupNameResolverAdapter( - DnsAddressResolverGroup resolverGroup, EventLoopGroup group) { - super(ImmediateEventExecutor.INSTANCE); - this.resolverGroup = resolverGroup; - this.group = group; - } - - @Override - protected void doResolve(String inetHost, Promise promise) throws Exception { - EventExecutor executor = this.findExecutor(); - resolverGroup.getResolver(executor).resolve(InetSocketAddress.createUnresolved(inetHost, 17)) - .addListener((FutureListener) future -> { - if (future.isSuccess()) { - promise.trySuccess(future.getNow().getAddress()); - } else { - promise.tryFailure(future.cause()); - } - }); - } - - @Override - protected void doResolveAll(String inetHost, Promise> promise) - throws Exception { - EventExecutor executor = this.findExecutor(); - resolverGroup.getResolver(executor).resolveAll(InetSocketAddress.createUnresolved(inetHost, 17)) - .addListener((FutureListener>) future -> { - if (future.isSuccess()) { - List addresses = new ArrayList<>(future.getNow().size()); - for (InetSocketAddress address : future.getNow()) { - addresses.add(address.getAddress()); - } - promise.trySuccess(addresses); - } else { - promise.tryFailure(future.cause()); - } - }); - } - - private EventExecutor findExecutor() { - EventExecutor current = ThreadExecutorMap.currentExecutor(); - return current == null ? group.next() : current; - } -} diff --git a/proxy/src/main/java/com/velocitypowered/proxy/network/netty/SeparatePoolInetNameResolver.java b/proxy/src/main/java/com/velocitypowered/proxy/network/netty/SeparatePoolInetNameResolver.java new file mode 100644 index 000000000..859bfa49b --- /dev/null +++ b/proxy/src/main/java/com/velocitypowered/proxy/network/netty/SeparatePoolInetNameResolver.java @@ -0,0 +1,79 @@ +package com.velocitypowered.proxy.network.netty; + +import com.google.common.util.concurrent.ThreadFactoryBuilder; +import io.netty.resolver.AddressResolver; +import io.netty.resolver.AddressResolverGroup; +import io.netty.resolver.DefaultNameResolver; +import io.netty.resolver.InetNameResolver; +import io.netty.util.concurrent.EventExecutor; +import io.netty.util.concurrent.Future; +import io.netty.util.concurrent.Promise; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.util.List; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.RejectedExecutionException; + +public final class SeparatePoolInetNameResolver extends InetNameResolver { + + private final ExecutorService resolveExecutor; + private final InetNameResolver delegate; + private AddressResolverGroup resolverGroup; + + /** + * Creates a new instnace of {@code SeparatePoolInetNameResolver}. + * + * @param executor the {@link EventExecutor} which is used to notify the listeners of the {@link + * Future} returned by {@link #resolve(String)} + */ + public SeparatePoolInetNameResolver(EventExecutor executor) { + super(executor); + this.resolveExecutor = Executors.newSingleThreadExecutor( + new ThreadFactoryBuilder() + .setNameFormat("Velocity DNS Resolver") + .setDaemon(true) + .build()); + this.delegate = new DefaultNameResolver(executor); + } + + @Override + protected void doResolve(String inetHost, Promise promise) throws Exception { + try { + resolveExecutor.execute(() -> this.delegate.resolve(inetHost, promise)); + } catch (RejectedExecutionException e) { + promise.setFailure(e); + } + } + + @Override + protected void doResolveAll(String inetHost, Promise> promise) + throws Exception { + try { + resolveExecutor.execute(() -> this.delegate.resolveAll(inetHost, promise)); + } catch (RejectedExecutionException e) { + promise.setFailure(e); + } + } + + public void shutdown() { + this.resolveExecutor.shutdown(); + } + + /** + * Returns a view of this resolver as a AddressResolverGroup. + * + * @return a view of this resolver as a AddressResolverGroup + */ + public AddressResolverGroup asGroup() { + if (this.resolverGroup == null) { + this.resolverGroup = new AddressResolverGroup() { + @Override + protected AddressResolver newResolver(EventExecutor executor) { + return asAddressResolver(); + } + }; + } + return this.resolverGroup; + } +}