othiym23 / async-listener

polyfill version of the 0.11 version of the asyncListener API
https://www.npmjs.org/package/async-listener
BSD 2-Clause "Simplified" License
174 stars 52 forks source link

Incompatible with bluebird when set as global Promise. #121

Closed mrfelton closed 7 years ago

mrfelton commented 7 years ago

When used with bluebird, this error is thrown:

Error: the promise constructor cannot be invoked directly

Which comes from here: http://bluebirdjs.com/docs/error-explanations.html#error-the-promise-constructor-cannot-be-invoked-directly

Note there it says one of the causes of this is:

  1. You are trying to subclass Promise

Bluebird does not support extending promises this way. Instead, see scoped prototypes.>

It seems that in async-listener@0.6.6, it is now attempting to subclass the global Promise. So, if global Promise has been set as Bluebird this fails. async-listener@0.6.5 does not suffer from this problem.

watson commented 7 years ago

@mrfelton Thanks for reporting this issue. Subclassing Promise was a fix for #112, but it was obviously not intended to have this side-effect.

I'm currently not sure if there is an easy fix for this besides rolling back 1618234d538cd36e8c167a495820147577369866

cc @matthewloring @hayes @ofrobots

hayes commented 7 years ago

Interesting. Async listener should not wrap non-native promises. I think there is a check for that, but it appears to not be working. I would look into making that check work, rather than trying to figure out how to wrapping bluebird promises.

hayes commented 7 years ago

Rolling back my not be a bad idea in the mean time, I suspect that more people is bluebird is likely more popular than any library that subclasses promises

watson commented 7 years ago

@hayes Yes good idea, I'll roll back for now

mrfelton commented 7 years ago

Async listener should not wrap non-native promises. I think there is a check for that, but it appears to not be working.

Does that mean async-listener is not supposed to work with non-native promises? Or just that non-native promises don't need wrapping for some reason?

hayes commented 7 years ago

Non native promises are usually implemented using some other asynchronous api.to schedule work. This means that async listener hooks will already be fired. Some implementations I believe will implement their own queues and share async tasks from the real queues and execute multiple callbacks from their fake/custom task queues. In these cases it is likely/possible that async contexts could get confused. Async listener does not try to solve those types of issues (similar issues exist with connection pools), but consumers of the async listener API (apm agents, continuation-local-storage, etc) will generally add their own wrappers for these special cases. Often there is not a right way to handle these edge cases that applies to every use case, so the solution varies depending on what your using the app for

watson commented 7 years ago

@mrfelton it should work as it's just vanilla javascript, but bluebird might have custom callback queues that make it a little tricky depending on your use-case. Personally I've needed to modify bluebird slightly to work well for my needs.

watson commented 7 years ago

yes, @hayes explanation is spot on

watson commented 7 years ago

@mrfelton this have now been fixed in v0.6.7

mrfelton commented 7 years ago

Got it. Thanks for the detailed explanation @hayes . In my case, this module is being used as part of continuation-local-storage.

hayes commented 7 years ago

You'll probably want to install https://www.npmjs.com/package/cls-bluebird

hayes commented 7 years ago

Reopening because the fix was reverted due to an issue with bluebird

hayes commented 7 years ago

Reopened the wrong issue