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

Make it work with Async Hooks #414

Closed
wants to merge 2 commits into from
Closed

Conversation

kjarmicki
Copy link

This is the reimplementation of #404 using AsyncResource instead of promise wrappers, as @Qard advised.
Tests were taken in their entirety from the original PR.

lib/read.js Outdated Show resolved Hide resolved
@dougwilson dougwilson added the pr label Oct 2, 2020
lib/read.js Show resolved Hide resolved
lib/read.js Show resolved Hide resolved
@edingroot
Copy link

edingroot commented Mar 13, 2022

May I know why this PR was closed?
I have the same problem that got undefined context created by AsyncLocalStorage when the request body is not empty (body-parser is called).

@kjarmicki
Copy link
Author

@edingroot body-parser is using raw-body and on-finished which are the lowest level modules that could have the fix applied and proliferate upwards from there, so it made sense to introduce it at the level of these modules.
I'm happy to say that there PRs were already merged 🙂 see stream-utils/raw-body#71 and jshttp/on-finished#36

I'm not sure when there's going to be an Express update that transitively pulls these changes in, but if you're in a hurry I think you could tackle it with npm overrides or patch-package.

@dougwilson
Copy link
Contributor

I am working on integrating them in to the very next body-parser release. There was a reported potential security issue with body-parser I am investigating is the only hold up. I pushed up the necessary changes, but rolled them back in anticipation of a patch release if this turns out to indeed be a security issue.

Everything should be done this week.

@edingroot
Copy link

Got it! Thank you all for the great work :)

@dougwilson
Copy link
Contributor

The changes are staged on master now if anyone wants to test, but will be released in the coming days :) !

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

Successfully merging this pull request may close these issues.

4 participants