[v8.x] http: attach reused parser to correct domain #25459

misterdjules · 2019-01-11T23:38:10Z

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

Reused parsers can be attached to the domain that corresponds to the
active domain when the underlying socket was created, which is not
necessarily correct.

Instead, we attach parsers to the active domain if there is one when
they're reused from the pool.

Refs: #25456

Reused parsers can be attached to the domain that corresponds to the active domain when the underlying socket was created, which is not necessarily correct. Instead, we attach parsers to the active domain if there is one when they're reused from the pool. Refs: nodejs#25456

nodejs-github-bot · 2019-01-11T23:38:11Z

@misterdjules sadly an error occured when I tried to trigger a build :(

misterdjules · 2019-01-11T23:46:34Z

/cc @nodejs/domains

addaleax

I’m not having an easy time figuring out why adding the parser to the current domain fixes things, and more importantly, why the domain module’s init() hook does not already add the parser to the domain?

addaleax · 2019-01-12T17:37:50Z

test/parallel/test-http-parser-reuse-domain.js

+        // Since this request throws before its callback has the chance
+        // to be called, we mark the test as failed if this callback is
+        // called.
+        process.exit(1);


If the only reason for this is to mark the test as failed, I’d prefer setting process.exitCode = 1 and letting the test finish on its own (here and elsewhere) – it might also be nice to get some output from the test in those cases?

Sure, i'll do both!

addaleax · 2019-01-12T17:41:36Z

test/parallel/test-http-parser-reuse-domain.js

+const agent = new http.Agent({
+  // keepAlive is important here so that the underlying socket of all requests
+  // is reused and tied to the same domain.
+  keepAlive: true


Can we have an assertion that checks that the connection is the same for all requests?

Good point!

Instead of an assertion, which can throw an exception and thus trigger a different code path with regards to domain error handling that would make this test really difficult to write, do you mind if, as for other failure cases in this test, we set the exit code to 1?

@misterdjules Yes, that sounds good to me as well :)

misterdjules · 2019-01-14T18:34:43Z

I’m not having an easy time figuring out why adding the parser to the current domain fixes things

Adding the parser to the current domain fixes things because by doing that, all events emitted on that parser will enter that domain. This is equivalent to what's done in node >= 10.x by the domain module's async hooks.

more importantly, why the domain module’s init() hook does not already add the parser to the domain?

This PR is targeted at the v8.x-staging branch where the domain module doesn't use async hooks to track resources lifecycle. The issue referenced by this PR specifically states:

This problem doesn't surface in node 10 because when a parser is reused, its underlying async resource is reinitialized and the proper async hooks events are emitted, which allows the domains state machine to associate the parser to the active domain.

addaleax

Thanks, I missed that this targets v8.x.

misterdjules · 2019-01-14T21:17:05Z

@addaleax Pushed a new commit that I think addresses your comments, thanks for the help!

misterdjules · 2019-01-14T21:59:25Z

I just noticed something else that seems to be an issue, but I think may be orthogonal to the problem that this PR is addressing: when the 'end' event is emitted on a client response/http parser instance from within a nextTick callback, the domains stack contains the domain to which the parser is associated more than once.

It means that if an error is thrown and not caught when the parser's 'end' listener is called, the 'error' listener of the active domain might be called more than once if it throws an error (since it's present on the domains stack more than once).

Again, I think this is orthogonal to the issue that this PR is fixing, so I'll open a separate issue to discuss that.

misterdjules · 2019-01-15T01:17:18Z

I just noticed something else that seems to be an issue, but I think may be orthogonal to the problem that this PR is addressing: [..] I'll open a separate issue to discuss that.

FWIW, issue opened at #25505.

MylesBorins · 2019-01-15T05:28:23Z

We are planning one last semver-minor with 8.x, so that would be a last chance to get this in. Does the bug only occur on that branch?

misterdjules · 2019-01-15T06:03:16Z

Does the bug only occur on that branch?

It occurs on the v8.x branch and previous branches like v6.x, but it doesn't occur on v10.x and "later" branches.

misterdjules · 2019-01-15T17:41:20Z

@MylesBorins I would think that at the very least we'd want to run citgm for these changes, is that something that will be done for the whole upcoming 8.x release, or should we do it separately for this change?

MylesBorins · 2019-01-15T18:36:25Z

@misterdjules we do run it on every release, but if you think this specific change should be checked I'm 100% in support

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1717/

misterdjules · 2019-01-15T19:49:27Z

@MylesBorins Excellent, thank you! I'll watch the results of that job.

misterdjules · 2019-01-16T00:47:22Z

@MylesBorins It seems that there's a lot of errors like:

 Error: Unable to parse: v8.15.1-pre

in https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1717/ from tape, so maybe we'll get meaningful results only when we run the citgm on an actual release build?

sbehrens · 2019-01-30T15:42:50Z

Hello can someone provide an update on when this PR will be merged? Thanks!

-Scott

misterdjules · 2019-01-30T18:23:29Z

@MylesBorins

We are planning one last semver-minor with 8.x

Do you know when that is planned (if planned) and whether this change would make it to that release? I've been following nodejs/Release#405 where you wrote:

I'm not in a rush tbqh, maybe end of feb / march

Is it still what you have in mind for the next 8.x release?

MylesBorins · 2019-01-30T18:34:06Z

another CI run https://ci.nodejs.org/job/node-test-pull-request/20451/
another CITGM run https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1731/

Late march is the plan for 8.x semver-minor, it will definitely be happeningg

MylesBorins · 2019-01-30T21:17:20Z

CI is green. Two failues in CITGM I don't recognize if you could take a look

MylesBorins · 2019-01-30T21:17:34Z

got on osx

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1731/nodes=osx1011/testReport/(root)/citgm/got_v9_6_0/

ava on ubuntu 12

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1731/nodes=ubuntu1204-64/testReport/(root)/citgm/ava_v1_2_0/

misterdjules · 2019-02-04T22:59:01Z

@MylesBorins I ran the following for both ava at version 1.2.0 and got at version 9.6.0 locally on my OS X setup:

$ ~/dev/node/node `which npm` run test --scripts-prepend-node-path

with ~/dev/node/node being a custom node binary built with the changes in this PR, and all tests passed. So I don't know what the citgm failures are, but it seems that could be a false positive due to how the test runs on the CI platform?

BethGriggs · 2019-03-20T17:09:33Z

got and ava are now known CITGM failures and not specific to this PR.
CI: https://ci.nodejs.org/job/node-test-commit/26906/ ✅

Reused parsers can be attached to the domain that corresponds to the active domain when the underlying socket was created, which is not necessarily correct. Instead, we attach parsers to the active domain if there is one when they're reused from the pool. Refs: #25456 PR-URL: #25459 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BethGriggs · 2019-03-20T17:10:37Z

Landed on v8.x-staging

misterdjules added http Issues or PRs related to the http subsystem. domain Issues and PRs related to the domain subsystem. v8.x labels Jan 11, 2019

addaleax reviewed Jan 12, 2019

View reviewed changes

addaleax changed the title ~~http: attach reused parser to correct domain~~ [v8.x] http: attach reused parser to correct domain Jan 14, 2019

addaleax approved these changes Jan 14, 2019

View reviewed changes

changes according to latest code review

c1004ad

get rid of dead code

2e459d0

BridgeAR approved these changes Jan 15, 2019

View reviewed changes

misterdjules mentioned this pull request Feb 13, 2019

Active Domain context is being leaked between server requests. #26081

Closed

rvagg force-pushed the v8.x-staging branch from 03e2a82 to d4b6643 Compare February 28, 2019 12:40

BethGriggs closed this Mar 20, 2019

MylesBorins mentioned this pull request Mar 26, 2019

v8.16.0 proposal #26933

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v8.x] http: attach reused parser to correct domain #25459

[v8.x] http: attach reused parser to correct domain #25459

misterdjules commented Jan 11, 2019

nodejs-github-bot commented Jan 11, 2019

misterdjules commented Jan 11, 2019

addaleax left a comment

addaleax Jan 12, 2019

misterdjules Jan 14, 2019

addaleax Jan 12, 2019

misterdjules Jan 14, 2019

addaleax Jan 14, 2019

misterdjules commented Jan 14, 2019

addaleax left a comment

misterdjules commented Jan 14, 2019

misterdjules commented Jan 14, 2019

misterdjules commented Jan 15, 2019

MylesBorins commented Jan 15, 2019

misterdjules commented Jan 15, 2019 •

edited

Loading

misterdjules commented Jan 15, 2019

MylesBorins commented Jan 15, 2019

misterdjules commented Jan 15, 2019

misterdjules commented Jan 16, 2019

sbehrens commented Jan 30, 2019

misterdjules commented Jan 30, 2019

MylesBorins commented Jan 30, 2019

MylesBorins commented Jan 30, 2019

MylesBorins commented Jan 30, 2019

misterdjules commented Feb 4, 2019

BethGriggs commented Mar 20, 2019

BethGriggs commented Mar 20, 2019

[v8.x] http: attach reused parser to correct domain #25459

[v8.x] http: attach reused parser to correct domain #25459

Conversation

misterdjules commented Jan 11, 2019

Checklist

nodejs-github-bot commented Jan 11, 2019

misterdjules commented Jan 11, 2019

addaleax left a comment

Choose a reason for hiding this comment

addaleax Jan 12, 2019

Choose a reason for hiding this comment

misterdjules Jan 14, 2019

Choose a reason for hiding this comment

addaleax Jan 12, 2019

Choose a reason for hiding this comment

misterdjules Jan 14, 2019

Choose a reason for hiding this comment

addaleax Jan 14, 2019

Choose a reason for hiding this comment

misterdjules commented Jan 14, 2019

addaleax left a comment

Choose a reason for hiding this comment

misterdjules commented Jan 14, 2019

misterdjules commented Jan 14, 2019

misterdjules commented Jan 15, 2019

MylesBorins commented Jan 15, 2019

misterdjules commented Jan 15, 2019 • edited Loading

misterdjules commented Jan 15, 2019

MylesBorins commented Jan 15, 2019

misterdjules commented Jan 15, 2019

misterdjules commented Jan 16, 2019

sbehrens commented Jan 30, 2019

misterdjules commented Jan 30, 2019

MylesBorins commented Jan 30, 2019

MylesBorins commented Jan 30, 2019

MylesBorins commented Jan 30, 2019

misterdjules commented Feb 4, 2019

BethGriggs commented Mar 20, 2019

BethGriggs commented Mar 20, 2019

misterdjules commented Jan 15, 2019 •

edited

Loading