Skip to content

Instantly share code, notes, and snippets.

@tabatkins
Created August 30, 2017 20:41
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save tabatkins/6f9e499ae902974a740f275900724adc to your computer and use it in GitHub Desktop.
Save tabatkins/6f9e499ae902974a740f275900724adc to your computer and use it in GitHub Desktop.
Problems with .then() handling the lock stuff

Currently, lock.asyncHold() isn't specced to take any arguments. It just returns a promise, and you pass the code you want to operate under the lock to the .then() method. This is problematic - it requires promise subclassing, and has some unintuitive and probably unexpected behavior.

For example, this code doesn't lock anything at all:

const p = lock.asyncHold();
...never call .then()...

This is extremely different from lock.hold(cb);, which immediately acquires the lock (and calls the cb with it held).


Similarly, this code doesn't lock anything either:

const p = lock.asyncHold();
const resultP = Promise.prototype.then.call(p, cb);

...because it uses the base Promise#then() function, which doesn't have any special "acquire the lock before running the cb" semantics. This is why I say this functionality requires Promise subclassing - you're not using the base Promise#then function.


Here's another unintuitive/unexpected bit of behavior:

const p = lock.asyncHold();
p.then(cb1);
p.then(cb2);

This acquires the lock twice, once for cb1 and once for cb2, from a single asyncHold() call. This is very different from how .hold() works, which acquires it only once.


The exact same arguments apply to asyncWait().

The core problem is that what you're looking for is a Task, not a Promise. Tasks are single-cast (can only call .then() on it once) and the Task can respond to the .then() being registered. (For example, they can delay actually doing some async work until something asks for it.) Passing the cb directly to asyncWait() is emulating the functionality that a Task would provide.

@othermaciej
Copy link

Assuming that it's necessary to subclass Promise (can't totally tell from this explanation why), why would that be a bad thing? Pulling out the 'then' method from the prototype doesn't seem likely. The fact that never using then() won't lock seems no different from what you propose with Task.

I'm also not sure why you think subclassing is necessary, but that could be just my ignorance. Would holding the lock around resolving the promise do the job? Why does then() need to do anything different? It just registers the callback, it does not invoke it (as I understand it). The way I understand the proposal, asyncHold() would imitate the process of asynchronously acquiring a lock right away, and once the lock is acquired (and you're back to the run loop) it resolves the promise with the lock held. This does mean that trying to then() more than once wouldn't do the right thing, so maybe explicitly passing a callback is better.

(All of this is a side issue to the main point of the post of course; clearly some async locking API could be made to work, even if the details are up for debate.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment