Good queries failing with "Response decode error" when running multiple requests in parallel via Amp\any - Fine when running in series about dns

[Daniel15]

Okay so what I think is happening here is that it's mixing the responses, and valid queries are somehow getting the error responses. Some of the responses are actually erroneous. If you run the requests in series by replacing the for loop with:

  for (
    $current_ip_raw = $start_ip_raw - IPS_TO_LOAD;
    $current_ip_raw < $start_ip_raw + IPS_TO_LOAD;
    $current_ip_raw++
  ) {
    $current_ip = long2ip($current_ip_raw);
    $ips[] = $current_ip;
    try {
      $succeeded[$current_ip] = (yield Amp\Dns\query($current_ip, Amp\Dns\Record::PTR));
    } catch (Exception $ex) {
      $failed[$current_ip] = $ex;
    }
  }

and removing the list($failed, $succeeded) = (yield Amp\any($lookups)); line, you'll see that one request constantly fails. If you change IPS_TO_LOAD to 10, you'll see two requests consistently fail (209.141.56.28 and 209.141.56.35). However, when running these requests in parallel via Amp\any, other requests also fail.

[kelunik]

This is still an issue in the Amp v2 version, additionally we leak the exception there.

<?php

use Amp\Loop;

require __DIR__ . '/vendor/autoload.php';

function format_exception(Exception $ex, $indent = 0) {
    $output = get_class($ex) . ' - ' . $ex->getMessage();
    if ($ex instanceof Amp\CombinatorException) {
        foreach ($ex->getExceptions() as $inner) {
            $output .= "\n" . str_repeat('  ', $indent) . ' --> ' . format_exception($inner, $indent + 1);
        }
    } else {
        $prev = $ex->getPrevious();
        if ($prev) {
            $output .= "\n" . str_repeat('  ', $indent) . ' --> ' . format_exception($prev, $indent + 1);
        }
    }
    return $output;
}

const IPS_TO_LOAD = 2;

Loop::run(function () {
    $start_ip = '209.141.56.29';
    $start_ip_raw = ip2long($start_ip);

    for (
        $current_ip_raw = $start_ip_raw - IPS_TO_LOAD;
        $current_ip_raw < $start_ip_raw + IPS_TO_LOAD;
        $current_ip_raw++
    ) {
        $current_ip = long2ip($current_ip_raw);
        $ips[] = $current_ip;
        $lookups[$current_ip] = Amp\Dns\query($current_ip, Amp\Dns\Record::PTR);
    }

    list($failed, $succeeded) = (yield Amp\Promise\any($lookups));
    echo '**** ', count($failed), " failures! ****\n\n";
    foreach ($ips as $ip) {
        if (array_key_exists($ip, $succeeded)) {
            $result = $succeeded[$ip][0]->getValue();
            echo $ip, ': ', $result, "\n";
        } else if (array_key_exists($ip, $failed)) {
            $ex = $failed[$ip];
            if ($ex instanceof Amp\Dns\NoRecordException) {
                echo $ip, ": No reverse DNS\n";
            } else {
                echo $ip, ': EXCEPTION: ', format_exception($ex), "\n";
            }
        } else {
            echo $ip, ": UNKNOWN\n";
        }
    }
});

[kelunik]

Okay so what I think is happening here is that it's mixing the responses, and valid queries are somehow getting the error responses. Some of the responses are actually erroneous.

This is nothing caused by our library, it's the server responding with an empty domain name.

[bwoebi]

I see no problems in the server response.
The failure is here or within libdns - I'm looking.

[kelunik]

@bwoebi It's in LibDNS.

[kelunik]

@bwoebi Seems like empty entries are indeed disallowed.

[kelunik]

Don't remember in which RFC I read that they're disallowed, can't find it again.

@DaveRandom ping.

[kelunik]

You sometimes get empty results now, but no more decode exceptions.

[DaveRandom]

LibDNS is being majorly refactored, one of the things that it will do is cease to validate things like this - i.e. context-dependent things (a domain name consisting of only the empty top-level label is, I suppose, theoretically valid).

In a case like this, however, I think it still makes sense for an exception to be throw by amp/dns (i.e. the client implementation). An empty domain name in response to a PTR query definitely does not make sense and is exceptional, even if it is dependent on external data.

One could perhaps have this behaviour controlled by an option to convert the response into the same behaviour as "non-existent record" when talking to buggy servers, but the default behaviour should be to throw. IMHO.