src: do not reuse HTTPParser by Drieger · Pull Request #25094 · nodejs/node (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

cc @nodejs/async_hooks @nodejs/diagnostics

Choose a reason for hiding this comment

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

LGTM

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?

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:

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

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?

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).

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.

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

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]);

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.

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.

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.

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.

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.

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.

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

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,

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?

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.

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

mcollina added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Mar 7, 2019

Choose a reason for hiding this comment

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

LGTM

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.

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 mat@mmarchini.me

PR-URL: #25094 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Benedikt Meurer benedikt.meurer@gmail.com Reviewed-By: Anna Henningsen anna@addaleax.net

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.

This was referenced

Apr 23, 2019

This was referenced

Apr 23, 2019

This was referenced

Apr 28, 2019

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 anna@addaleax.net Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Rich Trott rtrott@gmail.com

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

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 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 anna@addaleax.net Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Rich Trott rtrott@gmail.com

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 anna@addaleax.net Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Rich Trott rtrott@gmail.com

Reviewers

mcollina mcollina approved these changes

AndreasMadsen AndreasMadsen left review comments