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

support async hooks #88

Merged
merged 3 commits into from
Jun 11, 2020
Merged

support async hooks #88

merged 3 commits into from
Jun 11, 2020

Conversation

achille-roussel
Copy link

In places where we use async_hooks, the propagation of the async execution context stops when async functions returning thenable objects are called. This is because the async function will chain the call to the thenable, but it happens asynchronously on the process' next tick, which is outside the scope of the async execution context that originally called the function.

The superagent request objects we use in integrations are thenable, which exposes them to this issue. This means that we cannot use async local storage to propagate values from the incoming to outgoing requests.

Full details on the topic can be found in nodejs/node#22360

This PR addresses the issue by explicitly creating an async resource for each request that the integration creates, capturing the current execution context, and restoring it when the then method is eventually called.

Copy link

@pelletier pelletier left a comment

Choose a reason for hiding this comment

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

Props on digging that up!

// it via a call to runInAsyncScope in the overriden "then" function.
const asyncResource = new AsyncResource('com.segment.integration.Request')
const thenFunction = Reflect.get(req, 'then')
const thenAsyncHook = function then (resolve, reject) {
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Is the current implementation different than:

Suggested change
const thenAsyncHook = function then (resolve, reject) {
function thenAsyncHook (resolve, reject) {

Copy link
Author

Choose a reason for hiding this comment

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

I think in the current code the function will be named then, so it leaks less details about the hack we're doing.

const thenAsyncHook = function then () {}
console.log(thenAsyncHook.name)
console.log((function thenAsyncHook () {}).name)
$ node test.js
then
thenAsyncHook

Copy link
Member

Choose a reason for hiding this comment

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

Interesting thanks!

Copy link
Member

@mericsson mericsson left a comment

Choose a reason for hiding this comment

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

Very interesting. I'm not familiar with async_hooks but this lgtm.

7.1.0 package semver is good too.

you can merge and then npm publish.

then please yarn install @segment/[email protected] --exact in analytics-cloud-integrations and integrations.

@achille-roussel
Copy link
Author

Thank you all for the reviews!

@achille-roussel achille-roussel merged commit 3a87b9b into master Jun 11, 2020
@achille-roussel achille-roussel deleted the support-async-hooks branch June 11, 2020 20:55
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.

4 participants