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. (/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: https://github.com/othiym23/async-listener/blob/3e29e8cf645c732a2a4f833bd7c3d9df67936304/index.js#L331

But at this point the wrapPromise() function have already been called here: https://github.com/othiym23/async-listener/blob/3e29e8cf645c732a2a4f833bd7c3d9df67936304/index.js#L388

[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 notifications@github.com 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 https://github.com/othiym23/async-listener/issues/123#issuecomment-310787765, 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.

[watson]

@matthewloring I asked the same question a while back. Seems like some implementations would fake the toString function to return this sting. But I'm not sure if this is still the case.

[matthewloring]

Ah interesting. I would be good to know what polyfills replace this toString. This check works for promise, bluebird, and q at least at the latest versions. @hayes Do you remember which polyfill had the native toString behavior?

[matthewloring]

I got a chance to talk to the V8 team a bit about this and there is no way outside of native code to determine definitively that something is a native promise (there is %IsPromise but that requires starting node with the natives syntax flag). I'm not sure we'll be able to do better than a hack without adding a native component or dependency to this module. Maybe we would have higher assurance with:

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

@hayes @watson @Qard What do you think?

[ofrobots]

@hayes @watson @Qard any opinions? It would be good to make forward progress on this.

[othiym23]

How does async_hooks deal with this? By hiding it behind the implementation wall? I haven't reviewed @addaleax's recent PRs related to promises closely enough to know.

[Qard]

Sorry, I missed this before. My github notifications got out of hand so I just did a mark-all-as-read recently. 😥

I'm 👍 on checking toString() and toString.toString() are both native.

[addaleax]

How does async_hooks deal with this? By hiding it behind the implementation wall?

I’m not sure I understand what you mean by that – if it answers your question, async_hooks can only deal with native Promises, it doesn’t care about what the global Promise object is.

[Qard]

async_hooks handled it on the native side, with PromiseHook, since we have async/await to worry about, which bypasses a bunch of the user-facing promise API. Simply patching the then method didn't work there.