async_hooks: fixup do not reuse HTTPParser by Flarna · Pull Request #27477 · nodejs/node · GitHub

Fix some issues introduced/not fixed via
#25094:

Init hook is not emitted for a reused HTTPParser
HTTPParser was still used as resource in init hook
type used in init hook was always HTTPINCOMINGMESSAGE even for client
requests
some tests have not been adapted to new resource names

With this change the async hooks init event is emitted during a call
to Initialize() as the type and resource object is available at this
time. As a result Initialize() must be called now which could be seen
as breaking change even HTTPParser is not part of documented API.

It was needed to put the ClientRequest instance into a wrapper object
instead passing it directly as async resource otherwise
test-domain-multi fails. I think this is because adding an EventEmitter
to a Domain adds a property 'domain' and the presence of this changes
the context propagation in domains.

Besides that tests still refering to resource HTTPParser have been
updated/improved.

Fixes: #27467
Fixes: #26961
Refs: #25094

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Fix some issues introduced/not fixed via #25094: * Init hook is not emitted for a reused HTTPParser * HTTPParser was still used as resource in init hook * type used in init hook was always HTTPINCOMINGMESSAGE even for client requests * some tests have not been adapted to new resource names With this change the async hooks init event is emitted during a call to Initialize() as the type and resource object is available at this time. As a result Initialize() must be called now which could be seen as breaking change even HTTPParser is not part of documented API. It was needed to put the ClientRequest instance into a wrapper object instead passing it directly as async resource otherwise test-domain-multi fails. I think this is because adding an EventEmitter to a Domain adds a property 'domain' and the presence of this changes the context propagation in domains. Besides that tests still refering to resource HTTPParser have been updated/improved. Fixes: #27467 Fixes: #26961 Refs: #25094

fyi @addaleax @Drieger @mmarchini @mcollina @Trott I think you were involved in the first turn in this so would be nice if you could take a look here.

Some note regarding this change: I have not changed the structure that Parser is an AsyncWrap and reused, I just tried to get async hooks signaling correct events and unique resource objects. This is in my opinion kind of hacky.
A cleaner approach would be to don't reuse Parser but benchmarks in #24330 have shown that this has performance impact. I have never seen the benchmarks of the final change in #25094.
Maybe we should revisit a performance comparison as the current variant has the need to create two more objects compare to the initial approach.

CI: https://ci.nodejs.org/job/node-test-pull-request/22815/

Thanks for this! Is this believed to resolve the issue where test-async-hooks-http-parser-destroy sometimes fails?

@Trott From AsyncHooks point of view I think so. Not sure about the setTimeout in the test. I'm able to reproduce the fail from #26961 locally with version up to 12 but not with my local build.
Backporting of the fix may be a different story as this PR is a followup on others and not sure if they get backported down to Node 8.

Maybe we should increase N to 100 in the test?

Thanks, this is much nicer to review without the whitespace :)

LGTM

CI: https://ci.nodejs.org/job/node-test-pull-request/22877/

CI: https://ci.nodejs.org/job/node-test-pull-request/22883/ ✅

I don't think that my changes are the root cause for the failed CI build. Any hints how to continue here are welcome.

@Flarna The last CI run here was green, so I don’t think there’s an issue to be solved here. I’ll just go ahead and land this, thanks for the ping…

Landed in 8876ac5

Fix some issues introduced/not fixed via #25094: * Init hook is not emitted for a reused HTTPParser * HTTPParser was still used as resource in init hook * type used in init hook was always HTTPINCOMINGMESSAGE even for client requests * some tests have not been adapted to new resource names With this change the async hooks init event is emitted during a call to Initialize() as the type and resource object is available at this time. As a result Initialize() must be called now which could be seen as breaking change even HTTPParser is not part of documented API. It was needed to put the ClientRequest instance into a wrapper object instead passing it directly as async resource otherwise test-domain-multi fails. I think this is because adding an EventEmitter to a Domain adds a property 'domain' and the presence of this changes the context propagation in domains. Besides that tests still refering to resource HTTPParser have been updated/improved. Fixes: #27467 Fixes: #26961 Refs: #25094 PR-URL: #27477 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Relax the check regarding presence of async resources in graph to allow extra events. Before this change events not mentioned in reference graph were allowed but that one specified must match exactly in count. Now it's allowed to have more events of this type. Refs: nodejs#27477 Fixes: nodejs#27617

Relax the check regarding presence of async resources in graph to allow extra events. Before this change events not mentioned in reference graph were allowed but that one specified must match exactly in count. Now it's allowed to have more events of this type. Refs: nodejs#27477 Fixes: nodejs#27617 PR-URL: nodejs#27742 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Relax the check regarding presence of async resources in graph to allow extra events. Before this change events not mentioned in reference graph were allowed but that one specified must match exactly in count. Now it's allowed to have more events of this type. Refs: #27477 Fixes: #27617 PR-URL: #27742 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Avoid that destroy hook is invoked twice - once via `Parser::Free()` and once again via `Parser::Reinitialize()` by clearing the async_id in `EmitDestroy()`. Partial backport of nodejs#27477, a full backport would require also nodejs#25094 which has a dont-land-on-v10.x label on it. Fixes: nodejs#26961

Avoid that destroy hook is invoked twice - once via `Parser::Free()` and once again via `Parser::Reinitialize()` by clearing the async_id in `EmitDestroy()`. Partial backport of #27477, a full backport would require also #25094 which has a dont-land-on-v10.x label on it. Fixes: #26961 Backport-PR-URL: #27986 PR-URL: #27477 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 29, 2019

Flarna mentioned this pull request Apr 29, 2019

test: replace HTTPPARSER by new resource names #27372

Closed

3 tasks

Flarna added 2 commits April 29, 2019 15:30

make initialize() mandatory

40842c6

addaleax reviewed Apr 29, 2019

View reviewed changes

Comment thread src/async_wrap.h Outdated

Comment thread src/async_wrap.cc Outdated

Flarna added 2 commits April 29, 2019 16:04

fixup: revert whitespace changes

b4c189d

fixup: rename invalid_async_id to kInvalidAsyncId

a8bc5f1

Flarna commented Apr 29, 2019

View reviewed changes

Comment thread src/async_wrap.h

fixup: add initialize to DummyParser in test-http-parser-lazy-loaded

0d790d2

fixup: adapt bench-parser

565d515

addaleax approved these changes Apr 29, 2019

View reviewed changes

Comment thread src/async_wrap.h Outdated

mcollina approved these changes Apr 30, 2019

View reviewed changes

Trott approved these changes Apr 30, 2019

View reviewed changes

fixup: move reuse decission from AsyncReset to constructor

4677245

Flarna changed the title async_hooks: Fixup do not reuse HTTPParser async_hooks: fixup do not reuse HTTPParser May 2, 2019

addaleax closed this May 3, 2019

Flarna deleted the fix_parser_init branch May 3, 2019 15:12

Flarna mentioned this pull request May 8, 2019

http, http2: remove default server timeout #27558

Closed

3 tasks

ofrobots mentioned this pull request May 8, 2019

test/async-hooks/test-graph.http.js flaky #27617

Closed

Flarna mentioned this pull request May 10, 2019

async_hooks: destroy emitted more than once for an asyncId #26961

Closed

Flarna mentioned this pull request May 16, 2019

test: relax check in verify-graph #27742

Closed

2 tasks

Flarna mentioned this pull request May 30, 2019

[v10.x] async_hooks: avoid double-destroy HTTPParser #27986

Closed

2 tasks

BethGriggs mentioned this pull request Jul 17, 2019

v10.16.1 proposal #28731

Merged

Flarna mentioned this pull request Aug 7, 2019

process: improve nextTick performance #25461

Merged

2 tasks

Qard mentioned this pull request Sep 9, 2019

feat(http): add monitorIncomingHTTPRequests config elastic/apm-agent-nodejs#1298

Merged

3 tasks

Flarna mentioned this pull request Jan 28, 2020

async_hooks: add executionAsyncResource #30959

Closed

4 tasks

trentm mentioned this pull request Sep 30, 2021

new "run context" management to fix numerous span hierarchy issues elastic/apm-agent-nodejs#2181

Merged

5 tasks

molllyn1 mentioned this pull request Apr 15, 2024

[Snyk] Security upgrade gatsby from 2.18.18 to 2.24.80 molllyn1/node#110

Open

ron-a20 mentioned this pull request Apr 15, 2024

[Snyk] Security upgrade gatsby from 2.18.18 to 2.24.80 ron-a20/node#124

Open

kissofire mentioned this pull request Apr 15, 2024

[Snyk] Security upgrade gatsby from 2.13.73 to 2.24.80 kissofire/node#127

Open

137717unity mentioned this pull request Apr 15, 2024

[Snyk] Security upgrade gatsby from 2.18.18 to 2.24.80 137717unity/node#126

Open

aliceUnhinged613 mentioned this pull request Apr 15, 2024

[Snyk] Security upgrade gatsby from 2.18.18 to 2.24.80 aliceUnhinged613/node#126

Open

wedataintelligence mentioned this pull request Apr 16, 2024

[Snyk] Security upgrade gatsby from 2.18.18 to 2.24.80 wedataintelligence/node#109

Open

Flarna mentioned this pull request Apr 24, 2025

http: improve performance by removing async_hooks #57938

Closed

addaleax mentioned this pull request Aug 25, 2025

http: remove async tracking from http parser #59621

Closed

async_hooks: fixup do not reuse HTTPParser by Flarna · Pull Request #27477 · nodejs/node · GitHub

Uh oh!

Uh oh!

async_hooks: fixup do not reuse HTTPParser - #27477

async_hooks: fixup do not reuse HTTPParser#27477Flarna wants to merge 7 commits intonodejs:masternodejs/node:masterfrom Flarna:fix_parser_initCopy head branch name to clipboard

Conversation

Flarna commented Apr 29, 2019

Checklist

Uh oh!

Flarna commented Apr 29, 2019

Uh oh!

Uh oh!

Uh oh!

Flarna commented Apr 29, 2019

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 29, 2019

Uh oh!

Trott commented Apr 29, 2019

Uh oh!

Flarna commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 2, 2019

Uh oh!

nodejs-github-bot commented May 2, 2019 • edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Flarna commented May 3, 2019

Uh oh!

addaleax commented May 3, 2019

Uh oh!

addaleax commented May 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

async_hooks: fixup do not reuse HTTPParser#27477
Flarna wants to merge 7 commits into
nodejs:masterfrom
Flarna:fix_parser_init

Flarna commented Apr 29, 2019 •
edited
Loading

nodejs-github-bot commented May 2, 2019 •
edited by Trott
Loading