Resolving one promise from within another uses wrong storage

[watson]

trafficstars

I ran into an issue when resolving a promise from within the then callback of another promise. The issue can be recreated using the (over)simplified code below:

require('async-listener')

var cls = {n: 0}

process.addAsyncListener({
  create: function () {
    return {n: cls.n}
  },
  before: function (context, storage) {
    cls.n = storage.n
  }
})

var resolve
cls.n = 1
Promise.resolve().then(function then1 () {
  process._rawDebug('then1:', cls.n)
  resolve()
})

cls.n = 2
new Promise(function (_resolve) {
  resolve = _resolve
}).then(function then2 () {
  process._rawDebug('then2:', cls.n)
})

I expect the output of this to be:

then1: 1
then2: 2

But instead it's:

then1: 1
then2: 1

If the two promises are resolved individually it works as expected. But for some reason, the storage argument in the before hook of the ~~then1~~ (edit: meant then2) callback is {n: 1} when the 2nd promise is resolved from within the then1 callback.

[hayes]

This is working as intended. The reason for this is that the asynchronous boundary in this example is actually between the call to resolve and the callback passed to then, rather than between the call to then, and the execution of the callback.

In other words, the 2nd callback is triggered by resolving the promise in the first callback, and is therefore a continuation of that callback, and shares the same CLS context.

There are arguments to be made for it to work both ways, the current implementation is what most closely matches the behavior you would see if you used callbacks interested of promises to implement the same logic.

[watson]

Ok, I guess that can make sense.

My issue is that a more advanced version of this code unfortunately exists in the wild. Specifically in the latest major version of generic-pool which have now been converted to promises.

When a resource in the pool returns, generic-pool will now (inside a then callback) check if there's outstanding promises to be resolved in the queue. If there is, those promises will be resolved synchronously from within that then callback.

If this is not a bug, I guess the only solution is to monkey patch generic-pool.

[hayes]

The issues with resource pools is not specific to promises, and has been a major point of pain for anyone trying to implement monitoring solutions for node.js. Monkey patching has been the way it has been handled in the past when promised based apis were less common.

I think this issue does a good job of describing the different approaches that could be taken, and various tradeoffs involved. https://github.com/othiym23/node-continuation-local-storage/issues/64

With promises, and async-await becoming more and more common, this is likely a conversation that will continue to evolve. I have not been as involved in the tracing/monitoring community recently, so I am not sure what is being done in the area with asyncWrap, but that may be a good place to look for precedent.

[hayes]

Looks like async_wrap is heading in the same direction that we went for async listener. relevant issue is here: https://github.com/nodejs/promises/issues/9