Using forced Round Robin resolver with async-http-client

[Spikhalskiy]

Hi,

We need to have DNS resolution logic for our http client like here https://github.com/netty/netty/pull/5915/files#diff-0babf93c8bd17964680042f4442f38b5R36 (already merged to master), this is my PR to expose it as a NameResolver netty/netty#5915

So, we need to have an even distribution of connections/requests between addresses from DNS. But now async-http-client has very limited ability to implement this logic because it calls resolveAll() on NameResolver and pass it in NettyChannelConnector, where always starts with the top of the list. So this submitted RoundRobinInetAddressResolver or already existing RoundRobinInetSocketAddressResolver would not work as expected if passed to async-http-client request.

I think it would be great and flexible to rework this approach to always call resolve() in async-http-client before attempting to create one more connection, so a logic of connection distribution and host selection would be fully in a NameResolver implementation.

WDYT? Maybe you can recommend another way to fix/implement it?

[slandelle]

Hey,

Frankly, it's hard to keep up with upstream changes, eg I haven't backported this one atm.

I'd like to migrated Netty 4.1 ASAP but I'd like to fix #1280 and #1281 (I can do without, but it would be a real pity to keep on having my own components for things Netty "almost" does).

[Spikhalskiy]

Hi @slandelle, I will take a look if I can help with any of this. WS is not my proficiency for now, maybe would take a look at second one on a free time.

But I'm here mostly not about absence of forced RoundRobin in netty-bp, it's very easy to add with just

public class RoundRobinInetAddressResolver extends InetNameResolver {
    private NameResolver<InetAddress> nameResolver;

    /**
     * @param executor the {@link EventExecutor} which is used to notify the listeners of the {@link Future} returned by
     * {@link #resolve(java.net.SocketAddress)}
     * @param nameResolver the {@link NameResolver} used for name resolution
     */
    public RoundRobinInetAddressResolver(EventExecutor executor, NameResolver<InetAddress> nameResolver) {
        super(executor);
        this.nameResolver = nameResolver;
    }

    @Override
    protected void doResolve(String inetHost, Promise<InetAddress> promise) throws Exception {
        // hijack the doResolve request, but do a doResolveAll request under the hood
        // Note that InetSocketAddress.getHostName() will never incur a reverse lookup here,
        // because an unresolved address always has a host name.
        resolveAll(inetHost)
            .addListener((FutureListener<List<InetAddress>>) future -> {
                if (future.isSuccess()) {
                    List<InetAddress> inetAddresses = future.getNow();
                    int numAddresses = inetAddresses.size();
                    if (numAddresses == 0) {
                        promise.setFailure(new UnknownHostException(inetHost));
                    } else {
                        // if there are multiple addresses: we shall pick one at random
                        // this is to support the round robin distribution
                        int index =
                                (numAddresses == 1)? 0 : ThreadLocalRandom.current().nextInt(numAddresses);
                        promise.setSuccess(inetAddresses.get(index));
                    }
                } else {
                    promise.setFailure(future.cause());
                }
            });
    }

    @Override
    protected void doResolveAll(String inetHost, Promise<List<InetAddress>> promise) throws Exception {
        nameResolver.resolveAll(inetHost, promise);
    }
}

Problem is that async-http-client will not pick up this logic at all because of the method how it works with NameResolver. It would just call NameResolver#resolveAll and select a host from the top of the list and will completely ignore this random forced round robin logic in doResolve (NameResolver#resolve).

I will try to think and prepare some PR today to move to the NameResolver#resolve method (so, the fix would affect RequestHostnameResolver, NettyRequestSender, NettyChannelConnector) or maybe expose strategy selection as an option in RequestBuilder (looks like it would add get method to Request and break compatibility :(). So to discuss a possible fix.

[slandelle]

Why not just pass a custom NameResolver in the RequestBuilder?

[Spikhalskiy]

@slandelle This is what I do! But async-http-client only cares about NameResolver#resolveAlland completely ignores a logic in NameResolver#resolve, why instead it should use NameResolver#resolve to respect external NameResolver priority.

[slandelle]

Only resolveAll makes sense, because proper user agents implement failover strategies (when first address can't be reached, try the next one).

The problem is with your "round robin" strategy. You shouldn't be picking a random address, you should be shuffling the whole list.

[Spikhalskiy]

Only resolveAll makes sense, because proper user agents implement failover strategies (when first address can't be reached, try the next one).

This could be done in another way. For example, call resolve() for the next address and use resolveAll only if resolve call returned same result as a first one.
Or make it configurable - select host based on iterating thru resolveAll in case of fail or just a subsequent resolve calls.

The problem is with your "round robin" strategy. You shouldn't be picking a random address, you should be shuffling the whole list.

It's not mine, it's from netty's master. So, for now this forced round robin thing from netty is incompatible with async-http-client.
Plus, fact that async-http-client uses resolveAll and don't use resolve is pretty unobvious, should be bold javadoc about it. Because by default it should use resolve to respect full NameResolver selection logic.
From my point of view, it's pretty unobvious that a result of resolveAll should be shuffled for RoundRobin. resolveAll is just a resolve all, it's just a collection of all addresses. For prioritizing and selection of next address to use, NameResolver has resolve and I don't see reasons to don't use it.

WDYT?

[slandelle]

This could be done in another way. For example, call resolve() for the next address and use resolveAll only if resolve call returned same result as a first one.
Or make it configurable - select host based on iterating thru resolveAll in case of fail or just a subsequent resolve calls.

No, we don't want to perform name resolution again. We need the information that the name is mapped to multiple IPs. That's how proper user agents work.

It's not mine, it's from netty's master. So, for now this forced round robin thing from netty is incompatible with async-http-client.

Netty DNS stuff is experimental and unstable.

Plus, fact that async-http-client uses resolveAll and don't use resolve is pretty unobvious, should be bold javadoc about it. Because by default it should use resolve to respect full NameResolver selection logic.

Contribs welcome. I guess resolve is a thing for people who know for sure they only have a single IP and want to save array allocation, or who don't care about failover.
IMO, from a generic HTTP client pov, only resolveAll makes sense.

From my point of view, it's pretty unobvious that a result of resolveAll should be shuffled for RoundRobin. resolveAll is just a resolve all, it's just a collection of all addresses. For prioritizing and selection of next address to use, NameResolver has resolve and I don't see reasons to don't use it.

You can plug your own logic and decorate the way you want. That's what I do in Gatling.

[Spikhalskiy]

Closed by netty/netty#5972