Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

work around context loss with async-listener #21

Merged
merged 1 commit into from
Jun 21, 2017

Conversation

ofrobots
Copy link
Collaborator

async-listener is important module for APM tools and for long
stack traces. Promises make the concept of a long-stack-trace ambiguous
– as you can conceive the then callback as a continuation of either
the resolution context or the context that queued the then
callback (more details).

async-listener defaults to the resolution context. This is the wrong
default for how we are using promises here, resuling in APM tools
like Stackdriver Trace losing context.

We work-around the problem by not using the promise after it has
resolved.

R= @matthewloring

[async-listener][1] is important module for APM tools and for long
stack traces. Promises make the concept of a long-stack-trace ambiguous
– as you can conceive the `then` callback as a continuation of either
the resolution context or the context that queued the `then`
callback ([more details][2]).

async-listener defaults to the resolution context. This is the wrong
default for how we are using promises here, resuling in APM tools
like Stackdriver Trace losing context.

We work-around the problem by not using the promise after it has
resolved.

[1]: https://www.npmjs.com/package/async-listener
[2]: othiym23/node-continuation-local-storage#64
Copy link

@matthewloring matthewloring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully soon everything can just be async/await and we'll have async-hooks to prevent the need for these kinds of patterns.

@stephenplusplus
Copy link
Owner

This is some crazy stuff, but I trust you.

@stephenplusplus stephenplusplus merged commit 042ca58 into stephenplusplus:master Jun 21, 2017
@stephenplusplus
Copy link
Owner

Out in 0.7.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants