Unhandled promise rejection on lock timeout

[vincentwoo]

Examine the following code:

const AsyncLock = require("async-lock");
const lock = new AsyncLock({ timeout: 5000 });

lock.acquire("test", function(done) {}); // hold the lock open
const promise = lock.acquire("test", function() {}); // try to acquire the busy lock
promise.then(() => console.log("success"));
promise.catch(() => console.log("fail"));

What do you expect the program to output? I was expecting "fail", but the reality was:

$ node test.js
fail
(node:20581) UnhandledPromiseRejectionWarning: Error: async-lock timed out
    at Timeout._onTimeout (/home/vwoo/shared/autocomplete/node_modules/async-lock/lib/index.js:164:17)
    at ontimeout (timers.js:436:11)
    at tryOnTimeout (timers.js:300:5)
    at listOnTimeout (timers.js:263:5)
    at Timer.processTimers (timers.js:223:10)
(node:20581) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:20581) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

It is very strange to me that we have both the "fail" message as well as an unhandled promise rejection error. I'm not sure why and I'll try to take a peek inside the library to understand (just documenting my findings here).

[vincentwoo]

One thing that jumps out at me is that the library seems to have two different ideas of when it should be in "promise" mode or not.

/**
 * Acquire Locks
 *
 * @param {String|Array} key 	resource key or keys to lock
 * @param {function} fn 	async function
 * @param {function} cb 	callback function, otherwise will return a promise
 * @param {Object} opts 	options
 */

//... 

	if (typeof (cb) !== 'function') {
		opts = cb;
		cb = null;

		// will return a promise
		deferred =  new this.Promise(function(resolve, reject) {
			deferredResolve = resolve;
			deferredReject = reject;
		});
		deferred.catch(function() { console.log('wtf3') })
	}

The jsdoc claims that it is the presence of the cb function that triggers promise mode, which, hey, fair enough. However, elsewhere in the code:

		// Callback mode
		if (fn.length === 1) {
			var called = false;
			fn(function (err, ret) {
				if (!called) {
					called = true;
					done(locked, err, ret);
				}
			});
		}
		else {
			// Promise mode
			self._promiseTry(function () {
				return fn();
			})
			.then(function(ret){
				done(locked, undefined, ret);
			}, function(error){
				done(locked, error);
			});
		}

It seems the library also uses the signal of whether the fn executing function accepts a done callback parameter or not to determine if it is promise mode. This seems like a potential cause of this issue.

[vincentwoo]

Oh, maybe I don't have a bug, I forgot that promises fork in the chain - I should have written

promise.then(() => console.log("success")).catch(...

Closing for now!