src: do not reuse HTTPParser

[Drieger]

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 matheus@sthima.com

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

[mmarchini]

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

[mmarchini]

cc @nodejs/async_hooks @nodejs/diagnostics

[mcollina]

LGTM

[Drieger]

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]

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]

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]

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

[AndreasMadsen]

Thanks for spending time on this.

[Drieger]

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

[Flarna]

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

[Flarna]

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.

[Flarna]

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

[Drieger]

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

[Drieger]

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