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

async_hooks: fixup do not reuse HTTPParser #27477

Closed
wants to merge 7 commits into from

Conversation

@Flarna
Copy link
Member

commented Apr 29, 2019

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
Flarna added 2 commits Apr 22, 2019
async_hooks: fixup do not reuse HTTPParser
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

@Flarna Flarna force-pushed the Flarna:fix_parser_init branch from 5c027c2 to 40842c6 Apr 29, 2019

@Flarna

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

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.

src/async_wrap.h Outdated Show resolved Hide resolved
src/async_wrap.cc Outdated Show resolved Hide resolved
@Flarna

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

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.

src/async_wrap.h Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

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

@Flarna

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

@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?

@addaleax
Copy link
Member

left a comment

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

src/async_wrap.h Outdated Show resolved Hide resolved
@mcollina
Copy link
Member

left a comment

LGTM

@Trott
Trott approved these changes Apr 30, 2019
@nodejs-github-bot

This comment has been minimized.

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

@nodejs-github-bot

This comment has been minimized.

Copy link

commented May 2, 2019

@Flarna

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

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

@addaleax

This comment has been minimized.

Copy link
Member

commented May 3, 2019

@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…

@addaleax

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Landed in 8876ac5

@addaleax addaleax closed this May 3, 2019

addaleax added a commit that referenced this pull request May 3, 2019
async_hooks: fixup do not reuse HTTPParser
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>

@Flarna Flarna deleted the Flarna:fix_parser_init branch May 3, 2019

targos added a commit that referenced this pull request May 4, 2019
async_hooks: fixup do not reuse HTTPParser
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>
@targos targos referenced this pull request May 6, 2019
@Flarna Flarna referenced this pull request May 8, 2019
3 of 3 tasks complete
Flarna added a commit to dynatrace-oss-contrib/node that referenced this pull request May 16, 2019
test: relax check in verify-graph
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
@Flarna Flarna referenced this pull request May 16, 2019
2 of 2 tasks complete
pull bot pushed a commit to Rachelmorrell/node that referenced this pull request May 19, 2019
test: relax check in verify-graph
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>
targos added a commit that referenced this pull request May 20, 2019
test: relax check in verify-graph
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>
Flarna added a commit to dynatrace-oss-contrib/node that referenced this pull request May 30, 2019
[v10.x] backport avoid double-destroy HTTPParser
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
Flarna added a commit to dynatrace-oss-contrib/node that referenced this pull request May 30, 2019
[v10.x] backport avoid double-destroy HTTPParser
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
@Flarna Flarna referenced this pull request May 30, 2019
2 of 2 tasks complete
Flarna added a commit to dynatrace-oss-contrib/node that referenced this pull request May 30, 2019
async_hooks: avoid double-destroy HTTPParser
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
BethGriggs added a commit that referenced this pull request Jun 28, 2019
async_hooks: avoid double-destroy HTTPParser
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>
BethGriggs added a commit that referenced this pull request Jul 17, 2019
async_hooks: avoid double-destroy HTTPParser
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>
@BethGriggs BethGriggs referenced this pull request Jul 17, 2019
@Flarna Flarna referenced this pull request Aug 7, 2019
2 of 2 tasks complete
@Qard Qard referenced this pull request Sep 9, 2019
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.