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.

Sign up for GitHub

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

Jump to bottom

Fix context tracking with als run #3571

Merged
merged 3 commits into from Dec 23, 2021
Merged

Fix context tracking with als run #3571

merged 3 commits into from Dec 23, 2021

Conversation

mcollina
Copy link
Member

Fixes #3570

Checklist

@mcollina
Copy link
Member Author

@B4nan can you please verify this fixes the Nest.js problem?

@B4nan
Copy link

B4nan commented Dec 22, 2021

Amazing! Will verify this first thing in the morning, need to go AFK now.

zekth
zekth approved these changes Dec 22, 2021
View reviewed changes
Copy link
Member

@zekth zekth left a comment

Choose a reason for hiding this comment

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

Implementation is LGTM. Does it have any perf impact? i guess not.

@mcollina
Copy link
Member Author

It'd be minimal if at all. Once we drop Node v12 I can remove the 2 closures.

There is not much we can do about it anyway unless it's fixed in Node.js.

delvedor
delvedor approved these changes Dec 22, 2021
View reviewed changes
Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

RafaelGSS
RafaelGSS approved these changes Dec 22, 2021
View reviewed changes
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@climba03003 climba03003 added the benchmark Label to run benchmark against PR and main branch label Dec 22, 2021
@github-actions
Copy link

Node: 12
PR: [1] 1632k requests in 30.07s, 305 MB read
MAIN: [1] 1685k requests in 30.11s, 315 MB read

Node: 14
PR: [1] 1711k requests in 30.05s, 320 MB read
MAIN: [1] 1670k requests in 30.05s, 312 MB read

Node: 16
PR: [1] 1676k requests in 30.06s, 313 MB read
MAIN: [1] 1719k requests in 30.08s, 321 MB read

@github-actions github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Dec 22, 2021
@itheodoro itheodoro mentioned this pull request Dec 22, 2021
Closed
climba03003
climba03003 approved these changes Dec 23, 2021
View reviewed changes
Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

LGTM.

@mcollina mcollina merged commit fa432f0 into main Dec 23, 2021
@mcollina mcollina deleted the fix-context-tracking-with-als-run branch December 23, 2021 11:40
jsumners
jsumners reviewed Dec 23, 2021
View reviewed changes
lib/contentTypeParser.js
request.body = body
handler(request, reply)
}
// We cannot use resource.bind() because it is broken in node v12.
Copy link
Member

Choose a reason for hiding this comment

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

Probably should have a link to the issue here, but 🤷‍♂️

This was referenced Jan 25, 2022
Closed
Closed
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jsumners jsumners jsumners left review comments

@kibertoad kibertoad kibertoad approved these changes

@delvedor delvedor delvedor approved these changes

@zekth zekth zekth approved these changes

@climba03003 climba03003 climba03003 approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Async context is broken for POST requests when its created inside 'onRequest' hook
8 participants
@mcollina @B4nan @jsumners @kibertoad @delvedor @zekth @climba03003 @RafaelGSS