Errors during fallback are no longer handled

pimterry commented 3 years ago

In #43, it looks like we slightly changed the error handling behaviour. Repro:

const CacheableLookup = require('cacheable-lookup');

const cl = new CacheableLookup({
    lookup: (hostname, options, callback) => {
        setTimeout(() => {
            callback(Object.assign(new Error('Fake DNS error'), {
                code: 'EAI_AGAIN'
            }));
        }, 10);
    }
});

cl.lookup('example.test', (result) => {
    console.log('Got result', result);
});

In 6.0.1, this returns Error: cacheableLookup ENOTFOUND example.test In 6.0.2, this returns Error: Fake DNS error

This affects any cases where normal resolution fails with ENOTFOUND or ENODATA, and then fallback fails with some other error. Previously we treated that as an ENOTFOUND result, now we use the fallback's error instead.

I think this comes from c5d06c5 (#43), which removed the try/catch inside _lookup.

I don't think this is necessarily wrong, but it's a change in behaviour that I don't think was intentional. What should happen in this case?

szmarczak commented 3 years ago

This is a regression, will fix ASAP.

szmarczak commented 3 years ago

Well, to be honest we can either throw the lookup error or ENOTFOUND. The previous behavior was to ignore any lookup errors if real DNS queries were successful, which I'm not sure is the right behavior. Will do a release soon.

szmarczak commented 3 years ago

Released 6.0.3 :tada:

pimterry commented 3 years ago

:zap: amazingly fast fix, thanks! :smiley:

szmarczak / cacheable-lookup

Errors during fallback are no longer handled #48