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

src: do not reuse HTTPParser #25094

Closed
wants to merge 3 commits into from

Conversation

Drieger
Copy link
Contributor

@Drieger Drieger commented Dec 17, 2018

Change resource being used, previously HTTParser was being reused. We are now using IncomingMessage and ClientRequest objects. The goal here is to make the async resource unique for each async operation

Refs: #24330
Refs: nodejs/diagnostics#248
Refs: #21313

Co-authored-by: Matheus Marchini [email protected]

I continued @mmarchini work on #24330 and finished what was missing. I also added a test based on @AndreasMadsen comment nodejs/diagnostics#248 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot 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 Dec 17, 2018
@mmarchini
Copy link
Contributor

mmarchini commented Dec 17, 2018

@mmarchini
Copy link
Contributor

cc @nodejs/async_hooks @nodejs/diagnostics

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@Drieger
Copy link
Contributor Author

Drieger commented Dec 17, 2018

The problem with linter is that common is being imported but not used, but when I do not import common linter error is that it is mandatory. Any ideas how to solve this one?

@richardlau
Copy link
Member

richardlau commented Dec 17, 2018

The problem with linter is that common is being imported but not used, but when I do not import common linter error is that it is mandatory. Any ideas how to solve this one?

Just write:

require('../common');

or alternatively use common (e.g. common.mustCall).

@Flarna
Copy link
Member

Flarna commented Dec 17, 2018

If I understand this correct the resource type is still HTTPPARSER but depending on incoming/outgoing the resource passed in init differs. I think this is confusing.

Wouldn't it better to have two resource types instead?

@mscdex
Copy link
Contributor

mscdex commented Dec 18, 2018

Perhaps the commit message could be more clear. 'do not reuse HTTPParser' makes it sound as if parser objects are not being reused, when in fact it's the associated async resource that's no longer being reused (if I understand correctly).

Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

Thanks for spending time on this.

src/async_wrap.cc Show resolved Hide resolved
@Drieger
Copy link
Contributor Author

Drieger commented Dec 19, 2018

@Flarna maybe we could use IncomingMessage and ClientRequest instead? What do you think?

@Flarna
Copy link
Member

Flarna commented Dec 19, 2018

Not sure here; maybe prefix with Http as these names are quite generic in the global AsyncHooks scope.

parser.reinitialize(HTTPParser.REQUEST, parser[is_reused_symbol]);
parser.reinitialize(HTTPParser.REQUEST,
parser[is_reused_symbol],
server[kIncomingMessage]);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this reusing the IncomingMessage constructor function instead of the concrete instance? If I understand the code correct the IncomingMessage instance is created in parserOnHeadersComplete.
If I'm correct I wonder why the dedicated test-httparser-reuse.js is not detecting this.

Copy link
Member

Choose a reason for hiding this comment

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

@Drieger This one is still open and if I'm correct this is a no go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right @Flarna I'll investigate this more in depth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Flarna kIncomingMessage is really a function as you said. I fixed the test to catch this.
I will also create a new object to be the resource for the server. Some other changes will be necessary as well. I will update the PR as soon as I this is working.

@Drieger
Copy link
Contributor Author

Drieger commented Jan 9, 2019

Not sure here; maybe prefix with Http as these names are quite generic in the global AsyncHooks scope.

@Flarna I just updated the PR, adding two new providers HTTPINCOMINGMESSAGE and HTTPCLIENTREQUEST, could you take a look if that is what you had in mind?

I'll check why is the CI failing.

@Flarna
Copy link
Member

Flarna commented Jan 9, 2019

Yes, looks ok. I think an alternative would be HTTP_REQUEST/HTTP_RESPONSE like in code which would point to what happens instead naming the resource type used (currently).

I would love if others give also their comments on these names as they are actually public API (even it's just in experimental part).

Is HTTPPARSER still needed?

https://github.com/nodejs/node/blob/master/doc/api/async_hooks.md needs also some update once agreement on the names is reached.

@Drieger
Copy link
Contributor Author

Drieger commented Jan 9, 2019

You are right, I think HTTPPARSER is not necessary anymore. I'll update doc as soon as the "final" name is chosen.

@Drieger
Copy link
Contributor Author

Drieger commented Jan 10, 2019

Fix test issues!
I'll update commit message after all changes have been approved.

/cc @mmarchini @mcollina @AndreasMadsen @richardlau @mscdex could you check again? What do you think about the naming suggested by @Flarna?

@Drieger
Copy link
Contributor Author

Drieger commented Jan 14, 2019

I changed the resource in the _http_server.js file and fixed the test that was not able to capture the previous situation (function was being reused). I think it would be a good idea to run benchmarks if everyone agrees with the current implementation.

Obs.: Documentation and commit message will be updated if everything is ok.

// Force reinitilalization to make sure AsyncReset is called
parser.reinitialize(
HTTPParser.REQUEST,
true,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should remove this parameter, or is it used somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed parameter and renamed the method.

@mcollina
Copy link
Member

@mcollina
Copy link
Member

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/288/

(again, the previous one errored).

@Drieger
Copy link
Contributor Author

Drieger commented Mar 6, 2019

I think I addressed all previous comments, any other comments on this? @nodejs/diagnostics @nodejs/async_hooks

@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 7, 2019
Copy link
Member

@mcollina mcollina 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
Copy link
Member

mcollina commented Mar 7, 2019

I've tagged this semver-major because of the impact it can have on async_hooks users. It's not technically required because async_hooks are experimental, but they are widely used.
I'm fine in being semver-minor with a dont-land-on-8.x and dont-land-on-10.x label as well.

@mmarchini
Copy link
Contributor

Landing

@mmarchini
Copy link
Contributor

Landed in ece5073 🎉 🎉 🎉

A huge thanks for everyone who reviewed and worked on this!

mmarchini pushed a commit that referenced this pull request Apr 22, 2019
Change resource being used, previously HTTParser was being reused.
We are now using IncomingMessage and ClientRequest objects.  The goal
here is to make the async resource unique for each async operatio

Refs: #24330
Refs: nodejs/diagnostics#248
Refs: #21313

Co-authored-by: Matheus Marchini <[email protected]>

PR-URL: #25094
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@mmarchini mmarchini closed this Apr 22, 2019
@Flarna
Copy link
Member

Flarna commented Apr 22, 2019

I hoped that this one fixes #26961 but actually it didn't. I think that init hook is not called in case a Parser is reused since this change.

@refack refack mentioned this pull request Apr 23, 2019
3 tasks
addaleax pushed a commit that referenced this pull request May 3, 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

PR-URL: #27477
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request May 4, 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

PR-URL: #27477
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Flarna added a commit to dynatrace-oss-contrib/node that referenced this pull request May 30, 2019
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 pushed a commit that referenced this pull request Jun 28, 2019
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jul 17, 2019
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.