Reimplement ability to subclass Promise

[watson]

We need to reintroduce the ability to subclass Promise which was allowed by #113 but was reverted in #122. For this to happen we need to make sure we don't break bluebird as evident by #121.

[watson]

Here's a test case that will break when #113 is applied:

global.Promise = require('bluebird')
require('async-listener')
new Promise()

Stack trace from Bluebird

TypeError: the promise constructor cannot be invoked directly

    See http://goo.gl/MqrFmX

    at check (/Users/watson/code/node_modules/async-listener/node_modules/bluebird/js/release/promise.js:62:15)
    at WrappedPromise.Promise (/Users/watson/code/node_modules/async-listener/node_modules/bluebird/js/release/promise.js:72:9)
    at WrappedPromise (/Users/watson/code/node_modules/async-listener/es6-wrapped-promise.js:9:7)
    at Object.<anonymous> (/Users/watson/code/node_modules/async-listener/test/bluebird.js:3:1)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:605:10)
    at startup (bootstrap_node.js:158:16)
    at bootstrap_node.js:575:3

[hayes]

The correct behavior would be for async listener not to wrap non native promises

[watson]

A quick test reveals that this is a race condition. The guard actually correctly detects that it shouldn't instrument by ending up here:

async-listener/index.js

Line 331 in 3e29e8c

instrumentPromise = false;

But at this point the wrapPromise() function have already been called here:

async-listener/index.js

Line 388 in 3e29e8c

wrapPromise();

[hayes]

That is actually the intended behavior, that callback is being invoked asynchronously. What this means is either another async should have trigger (a few lines above) or bluebird is using an app not yet wrapped to schedule the async callback. That particular check was to handle bad implementations of promises that resolved a then synchronously

[watson]

Oh ignore my previous comment. I think I probably misinterpreted the "should not resolve synchronously" test at first glance. What this test does is checking if then calls its callback synchronously, in which case it's not considered a native promise. But apparently bluebird have become so good at impersonating native promises so that this guard doesn't detect it

[hayes]

Sorry for all the typos (I'm on my phone)

[watson]

@hayes haha yes you are right - GitHub needs a "is typing...". I just discovered the same thing as evident by my new comment 😄

[hayes]

Looks like bluebird uses native promises to schedule it's tasks: https://github.com/petkaantonov/bluebird/blob/master/src/schedule.js which we have not (and now cannot) wrap. This is really inconvenient.

[hayes]

The only way I see to fix this off the top of my head is to wrap bluebird as if it was native, not a big fan of that though

[watson]

Another option - which I'm not a big fan of either - is feature detection. The Bluebird Promise object have a few properties that the native promise doesn't

[hayes]

The thing I worry about is that promises still need to he wrapped. For an app using bluebird to work, we do need to wrap something

…

On Fri, Jun 23, 2017, 3:36 PM Thomas Watson ***@***.***> wrote: Another option - which I'm not a big fan of either - is feature detection. The Bluebird Promise object have a few properties that the native promise doesn't — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#123 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAx1jymQGauqvr0bh3qPqD1qcoaUPF8Wks5sHD3kgaJpZM4OEFlj> .

[hayes]

We could call into Promise.setScheduler if it exists and behaves like a native promise.

[hayes]

In that case, we could.fall back to something other than native promises as the asynchronizer

[matthewloring]

I'm not sure this module should patch non-native promise implementations as the patching is complex and can have unexpected consequences (a was the case with bluebird). Also, patching the bluebird scheduler would not give bluebird promises the same behavior as native promises to my knowledge which could make things more confusing as instead of better (there is extra logic to determine what context to propagate when then is called that would be missing among other things).

If we want to strengthen the native promise check, maybe we could add:

Promise.toString() === 'function Promise() { [native code] }'

I'm not aware of a check that would be complete but it seems unlikely that there would be many userspace C++ promise implementations.