child_process: allow 'http_parser' monkey patching again #24006

Jimbly · Oct 31, 2018

Lazy load _http_common and HTTPParser so that the 'http_parser' binding
can be monkey patched before any internal modules require it. This also
probably improves startup performance minimally for programs that never
require the HTTP stack.

Fixes: #23716
Fixes: creationix/http-parser-js#57

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Trott · Nov 4, 2018

@nodejs/child_process This could use some reviews.

Jimbly · Nov 4, 2018

If this fix is acceptable, and, additionally, something the Node team wants to support going forward, I'll add a test case ensuring http_parser is not required prior to user-level code being run, however I totally understand if that's not something the project wants to be held to in the future. Regardless, since this is a small fix, it would be great to get this merged to Node v10 to fix issues users of http-parser-js are running into.

joyeecheung · Nov 4, 2018

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

joyeecheung · Nov 4, 2018

joyeecheung approved these changes Nov 4, 2018

View changes

Regardless of the monkey-patchablility, it makes sense to lazy load the HTTP parser in child_process, so LGTM

jasnell · Nov 4, 2018

jasnell approved these changes Nov 4, 2018

View changes

cjihrig · Nov 4, 2018

cjihrig approved these changes Nov 4, 2018

View changes

If this fix is acceptable, and, additionally, something the Node team wants to support going forward, I'll add a test case ensuring http_parser is not required prior to user-level code being run

Yes, please add a test.

Jimbly · Nov 5, 2018

@cjihrig I've added a test (which fails on Node v10.1.0+, passes with this fix).

cjihrig · Nov 5, 2018

cjihrig approved these changes Nov 5, 2018

View changes

Thanks, LGTM with a couple optional comments.

cjihrig · Nov 5, 2018

test/parallel/test-http-parser-lazy-loaded.js

+// Test in a child process with IPC (specific case of https://github.com/nodejs/node/issues/23716)
+const child = fork(__filename, [ 'child' ]);
+child.on('exit', common.mustCall((code, signal) => {
+  assert.strictEqual(code, 0);


Can you also assert on signal being null.

cjihrig · Nov 5, 2018

test/parallel/test-http-parser-lazy-loaded.js

+
+// Test in our process
+doTest();
+// Test in a child process with IPC (specific case of https://github.com/nodejs/node/issues/23716)


You could execute this block of code only if process.argv[2] !== 'child', then you could get rid of the logic above, only calling doTest() once, unconditionally. Then, since it's only called once, you could get rid of doTest(), and inline its body 😄

Yeah, my first pass had the child process sending a message back to say things succeeded, so it was more complicated, and after I submitted my last change I did notice this could be simplified. Will do, since I'm making the other changes =).

cjihrig · Nov 5, 2018

cjihrig reviewed Nov 5, 2018

View changes

cjihrig · Nov 5, 2018

test/parallel/test-http-parser-lazy-loaded.js

+  }
+}
+DummyParser.REQUEST = Symbol();
+process.binding('http_parser').HTTPParser = DummyParser;


Oh, and I think this should be using internalBinding, since we recently moved toward gradually deprecating all of process.binding().

I saw that and tried internalBinding, but it complained with ReferenceError: internalBinding is not defined. Do you need to do something special to access this in tests and/or user-level code? I saw that 'http_parser' was on the 'process.binding white list' at some point, but I'm not sure if that's so that users can still get at it, or just a migration path toward deprecation.

Yes, put this at the top of the file:

// Flags: --expose-internals

That lets you specify command line arguments that the test needs.

Then:

const { internalBinding } = require('internal/test/binding');

Okay, that seems to work for the test. I'm assuming the best option is still using process.binding('http_parser') for users of monkey patching in http-parser-js, or is there any non-test way to get at internalBinding?

Thanks for the comments and assistance, I'm a complete novice at the Node testing framework ^_^. I've updated the test and will ping when it's cleared Travis CI and ready for final review / merge.

Jimbly · Nov 5, 2018

Updated test case code after comments from @cjihrig. I think is is now ready to go!

Trott · Nov 6, 2018

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

Trott · Nov 6, 2018

Looks like the test fails when run in a worker?

19:28:01 not ok 906 parallel/test-http-parser-lazy-loaded
19:28:01   ---
19:28:01   duration_ms: 0.515
19:28:01   severity: fail
19:28:01   exitcode: 1
19:28:01   stack: |-
19:28:01     (node:14125) internal/test/binding: These APIs are exposed only for testing and are not tracked by any versioning system or deprecation process.
19:28:01     internal/modules/cjs/loader.js:605
19:28:01         throw err;
19:28:01         ^
19:28:01     
19:28:01     Error: Cannot find module 'internal/test/binding'
19:28:01         at Function.Module._resolveFilename (internal/modules/cjs/loader.js:603:15)
19:28:01         at Function.Module._load (internal/modules/cjs/loader.js:529:25)
19:28:01         at Module.require (internal/modules/cjs/loader.js:658:17)
19:28:01         at require (internal/modules/cjs/helpers.js:22:18)
19:28:01         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/parallel/test-http-parser-lazy-loaded.js:5:29)
19:28:01         at Module._compile (internal/modules/cjs/loader.js:722:30)
19:28:01         at Object.Module._extensions..js (internal/modules/cjs/loader.js:733:10)
19:28:01         at Module.load (internal/modules/cjs/loader.js:620:32)
19:28:01         at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
19:28:01         at Function.Module._load (internal/modules/cjs/loader.js:552:3)
19:28:01     
19:28:01     events.js:167
19:28:01           throw er; // Unhandled 'error' event
19:28:01           ^
19:28:01     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
19:28:01     
19:28:01     1 !== 0
19:28:01     
19:28:01         at ChildProcess.child.on.common.mustCall (/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/parallel/test-http-parser-lazy-loaded.js:30:12)
19:28:01         at ChildProcess.<anonymous> (/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/common/index.js:340:15)
19:28:01         at ChildProcess.emit (events.js:182:13)
19:28:01         at Process.ChildProcess._handle.onexit (internal/child_process.js:254:12)
19:28:01     Emitted 'error' event at:
19:28:01         at Worker.[kOnErrorMessage] (internal/worker.js:332:10)
19:28:01         at Worker.[kOnMessage] (internal/worker.js:342:37)
19:28:01         at MessagePort.Worker.(anonymous function).on (internal/worker.js:279:57)
19:28:01         at MessagePort.emit (events.js:182:13)
19:28:01         at MessagePort.onmessage (internal/worker.js:84:8)
19:28:01   ...

Maybe someone in @nodejs/workers knows the right way to fix?

Jimbly · Nov 6, 2018

Does the the fork() call need --expose-internals passed manually to the node process? Any way to reproduce this locally?

Trott · Nov 6, 2018

Any way to reproduce this locally?

I think you can run tools/test.py --worker test/parallel/test-http-parser-lazy-loaded.js

Jimbly · Nov 6, 2018

Looks like it reproduced with running ./node --expose-internals --experimental-worker tools/run-worker.js test/parallel/test-http-parser-lazy-loaded.js. Problem appears to be that in a worker, forked processes are not inheriting the --expose-internals flag. Other tests seem to be calling spawn() instead of fork() and manually passing the --expose-internals to the child process, so I have updated my test to do that as well, and it seems to still work fine (fails on old Node, passes with my change, and now in a worker as well).

paulrutter · Nov 18, 2018

Do these messages mean it won't end up in 10.x?

Trott · Nov 19, 2018

Do these messages mean it won't end up in 10.x?

No, there just hasn't been a 10.x release proposed in the last couple weeks. I've added the lts-watch-v10.x label which I think explicitly requests that it be added to a 10.x release if at all possible. (@nodejs/releasers Please let me know if I'm using it wrong and only releasers should apply that label.)

rvagg · Nov 19, 2018

I'm don't have a good recent record for label usage but this seems appropriate to me

nodejs-github-bot added the child_process label Oct 31, 2018

Jimbly referenced this pull request Oct 31, 2018
Closed
No longer able to monkey-patch HTTP parser #23716

Jimbly force-pushed the Jimbly:patch-2 branch from cc2745c to db33bc5 Nov 5, 2018

Jimbly force-pushed the Jimbly:patch-2 branch from db33bc5 to 9984257 Nov 5, 2018

Jimbly force-pushed the Jimbly:patch-2 branch from 9984257 to 05bf434 Nov 5, 2018

Jimbly force-pushed the Jimbly:patch-2 branch from 05bf434 to abe4f50 Nov 5, 2018

Trott added the author ready label Nov 6, 2018

Jimbly force-pushed the Jimbly:patch-2 branch from abe4f50 to eb4cd15 Nov 6, 2018

BridgeAR referenced this pull request Nov 14, 2018
Merged
Release proposal: v11.2 #24350

Trott added the lts-watch-v10.x label Nov 19, 2018

codebytere added backported-to-v10.x and removed lts-watch-v10.x labels Dec 13, 2018

codebytere referenced this pull request Jan 4, 2019
Merged
v10.15.1 proposal #25346

BethGriggs referenced this pull request Mar 11, 2019
Open
Final 6.x release before EOL? [Discussion] #414

nodejs/node

Join GitHub today

child_process: allow 'http_parser' monkey patching again #24006

Conversation

Jimbly commented Oct 31, 2018 • edited

Checklist

nodejs-github-bot added the child_process label Oct 31, 2018

Jimbly referenced this pull request Oct 31, 2018

This comment has been minimized.

Trott commented Nov 4, 2018

This comment has been minimized.

Jimbly commented Nov 4, 2018

This comment has been minimized.

joyeecheung commented Nov 4, 2018

joyeecheung approved these changes Nov 4, 2018 View changes

joyeecheung left a comment

jasnell approved these changes Nov 4, 2018 View changes

cjihrig approved these changes Nov 4, 2018 View changes

cjihrig left a comment

Jimbly added a commit to Jimbly/node that referenced this pull request Nov 5, 2018

Jimbly force-pushed the Jimbly:patch-2 branch from cc2745c to db33bc5 Nov 5, 2018

Jimbly added a commit to Jimbly/node that referenced this pull request Nov 5, 2018

Jimbly force-pushed the Jimbly:patch-2 branch from db33bc5 to 9984257 Nov 5, 2018

Jimbly added a commit to Jimbly/node that referenced this pull request Nov 5, 2018

Jimbly force-pushed the Jimbly:patch-2 branch from 9984257 to 05bf434 Nov 5, 2018

Jimbly added a commit to Jimbly/node that referenced this pull request Nov 5, 2018

This comment has been minimized.

Jimbly commented Nov 5, 2018

cjihrig approved these changes Nov 5, 2018 View changes

cjihrig left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cjihrig reviewed Nov 5, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Jimbly force-pushed the Jimbly:patch-2 branch from 05bf434 to abe4f50 Nov 5, 2018

Jimbly added a commit to Jimbly/node that referenced this pull request Nov 5, 2018

This comment has been minimized.

Jimbly commented Nov 5, 2018

This comment has been minimized.

Trott commented Nov 6, 2018

Trott added the author ready label Nov 6, 2018

This comment has been minimized.

Trott commented Nov 6, 2018

This comment has been minimized.

Jimbly commented Nov 6, 2018

This comment has been minimized.

Trott commented Nov 6, 2018

Jimbly force-pushed the Jimbly:patch-2 branch from abe4f50 to eb4cd15 Nov 6, 2018

Jimbly added a commit to Jimbly/node that referenced this pull request Nov 6, 2018

This comment has been minimized.

Jimbly commented Nov 6, 2018

tadhgcreedon pushed a commit to tadhgcreedon/node that referenced this pull request Nov 11, 2018

BridgeAR added a commit that referenced this pull request Nov 14, 2018

BridgeAR referenced this pull request Nov 14, 2018

kiyomizumia added a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018

This was referenced Nov 15, 2018

This comment has been minimized.

paulrutter commented Nov 18, 2018

Trott added the lts-watch-v10.x label Nov 19, 2018

This comment has been minimized.

Trott commented Nov 19, 2018

This comment has been minimized.

rvagg commented Nov 19, 2018

codebytere added a commit that referenced this pull request Dec 13, 2018

codebytere added backported-to-v10.x and removed lts-watch-v10.x labels Dec 13, 2018

MylesBorins added a commit that referenced this pull request Dec 26, 2018

codebytere referenced this pull request Jan 4, 2019

This was referenced Jan 29, 2019

BethGriggs referenced this pull request Mar 11, 2019

Jimbly commented Oct 31, 2018 •

edited

joyeecheung approved these changes Nov 4, 2018

View changes

jasnell approved these changes Nov 4, 2018

View changes

cjihrig approved these changes Nov 4, 2018

View changes

Jimbly force-pushed the Jimbly:patch-2 branch from `cc2745c` to `db33bc5` Nov 5, 2018

Jimbly force-pushed the Jimbly:patch-2 branch from `db33bc5` to `9984257` Nov 5, 2018

Jimbly force-pushed the Jimbly:patch-2 branch from `9984257` to `05bf434` Nov 5, 2018

cjihrig approved these changes Nov 5, 2018

View changes

cjihrig reviewed Nov 5, 2018

View changes

Jimbly force-pushed the Jimbly:patch-2 branch from `05bf434` to `abe4f50` Nov 5, 2018

Jimbly force-pushed the Jimbly:patch-2 branch from `abe4f50` to `eb4cd15` Nov 6, 2018