Fix promise not being removed from queue on fallback query (#34)

Co-authored-by: Florian Gambert <florian.gambert@radiofrance.com>
Co-authored-by: Szymon Marczak <36894700+szmarczak@users.noreply.github.com>

source/index.js

@@ -206,7 +206,11 @@ class CacheableLookup {
 				const newPromise = this.queryAndCache(hostname);
 				this._pending[hostname] = newPromise;
 
-				cached = await newPromise;
+				try {
+					cached = await newPromise;
+				} finally {
+					delete this._pending[hostname];
+				}
 			}
 		}
 
@@ -320,29 +324,21 @@ class CacheableLookup {
 			return this._dnsLookup(hostname, all);
 		}
 
-		try {
-			let query = await this._resolve(hostname);
+		let query = await this._resolve(hostname);
 
-			if (query.entries.length === 0 && this._fallback) {
-				query = await this._lookup(hostname);
+		if (query.entries.length === 0 && this._fallback) {
+			query = await this._lookup(hostname);
 
-				if (query.entries.length !== 0) {
-					// Use `dns.lookup(...)` for that particular hostname
-					this._hostnamesToFallback.add(hostname);
-				}
+			if (query.entries.length !== 0) {
+				// Use `dns.lookup(...)` for that particular hostname
+				this._hostnamesToFallback.add(hostname);
 			}
+		}
 
-			const cacheTtl = query.entries.length === 0 ? this.errorTtl : query.cacheTtl;
-			await this._set(hostname, query.entries, cacheTtl);
-
-			delete this._pending[hostname];
-
-			return query.entries;
-		} catch (error) {
-			delete this._pending[hostname];
+		const cacheTtl = query.entries.length === 0 ? this.errorTtl : query.cacheTtl;
+		await this._set(hostname, query.entries, cacheTtl);
 
-			throw error;
-		}
+		return query.entries;
 	}
 
 	_tick(ms) {

tests/test.js

@@ -792,6 +792,53 @@ test.serial('fallback works', async t => {
 	});
 });
 
+test.serial('fallback works if ip change', async t => {
+	const cacheable = new CacheableLookup({resolver, fallbackDuration: 3600});
+	resolver.resetCounter();
+	resolver.lookupData.osHostnameChange = [
+		{address: '127.0.0.1', family: 4},
+		{address: '127.0.0.2', family: 4}
+	];
+
+	// First call: do not enter in `if (this._hostnamesToFallback.has(hostname)) {`
+	const entries = await cacheable.query('osHostnameChange', {all: true});
+	t.is(entries.length, 2);
+
+	t.is(entries[0].address, '127.0.0.1');
+	t.is(entries[0].family, 4);
+
+	t.is(entries[1].address, '127.0.0.2');
+	t.is(entries[1].family, 4);
+
+	t.is(cacheable._cache.size, 0);
+
+	// Second call: enter in `if (this._hostnamesToFallback.has(hostname)) {`
+	// And use _dnsLookup
+	// This call is used to ensure that this._pending is cleaned up when the promise is resolved
+	await cacheable.query('osHostnameChange', {all: true});
+
+	// Third call: enter in `if (this._hostnamesToFallback.has(hostname)) {`
+	// And use _dnsLookup
+	// Address should be different
+	resolver.lookupData.osHostnameChange = [
+		{address: '127.0.0.3', family: 4},
+		{address: '127.0.0.4', family: 4}
+	];
+	const entries2 = await cacheable.query('osHostnameChange', {all: true});
+
+	t.is(entries2.length, 2);
+
+	t.is(entries2[0].address, '127.0.0.3');
+	t.is(entries2[0].family, 4);
+
+	t.is(entries2[1].address, '127.0.0.4');
+	t.is(entries2[1].family, 4);
+
+	t.is(cacheable._cache.size, 0);
+
+	delete resolver.lookupData.osHostnameChange;
+});
+
 test('real DNS queries first', async t => {
 	const resolver = createResolver({delay: 0});
 	const cacheable = new CacheableLookup({