child_process: allow 'http_parser' monkey patching again #24006

Jimbly · 2018-10-31T21:03:45Z

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

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: nodejs#23716 Fixes: creationix/http-parser-js#57

Trott · 2018-11-04T02:28:39Z

@nodejs/child_process This could use some reviews.

Jimbly · 2018-11-04T03:01:44Z

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 · 2018-11-04T07:04:16Z

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

joyeecheung

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

cjihrig

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 · 2018-11-05T17:44:24Z

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

cjihrig

Thanks, LGTM with a couple optional comments.

cjihrig · 2018-11-05T20:43:57Z

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 · 2018-11-05T20:45:26Z

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 · 2018-11-05T20:46:44Z

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 · 2018-11-05T22:04:56Z

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

Trott · 2018-11-06T03:16:25Z

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

Trott · 2018-11-06T04:52:34Z

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 · 2018-11-06T06:01:51Z

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

Trott · 2018-11-06T06:36:36Z

Any way to reproduce this locally?

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

Jimbly · 2018-11-06T15:09:03Z

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

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 PR-URL: #24006 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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: nodejs#23716 Fixes: creationix/http-parser-js#57 PR-URL: nodejs#24006 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

paulrutter · 2018-11-18T20:36:07Z

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

Trott · 2018-11-19T03:41:44Z

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 · 2018-11-19T11:42:42Z

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

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 PR-URL: #24006 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Oct 31, 2018

Jimbly mentioned this pull request Oct 31, 2018

No longer able to monkey-patch HTTP parser #23716

Closed

joyeecheung approved these changes Nov 4, 2018

View reviewed changes

jasnell approved these changes Nov 4, 2018

View reviewed changes

cjihrig approved these changes Nov 4, 2018

View reviewed changes

Jimbly force-pushed the patch-2 branch from cc2745c to db33bc5 Compare November 5, 2018 16:25

Jimbly force-pushed the patch-2 branch from db33bc5 to 9984257 Compare November 5, 2018 16:31

Jimbly force-pushed the patch-2 branch from 9984257 to 05bf434 Compare November 5, 2018 16:52

cjihrig approved these changes Nov 5, 2018

View reviewed changes

cjihrig reviewed Nov 5, 2018

View reviewed changes

Jimbly force-pushed the patch-2 branch from 05bf434 to abe4f50 Compare November 5, 2018 21:19

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 6, 2018

Jimbly force-pushed the patch-2 branch from abe4f50 to eb4cd15 Compare November 6, 2018 15:08

BridgeAR mentioned this pull request Nov 14, 2018

Release proposal: v11.2 #24350

Merged

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 mentioned this pull request Jan 4, 2019

v10.15.1 proposal #25346

Merged

BethGriggs mentioned this pull request Mar 11, 2019

Final 6.x release before EOL? [Discussion] nodejs/Release#414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: allow 'http_parser' monkey patching again #24006

child_process: allow 'http_parser' monkey patching again #24006

Jimbly commented Oct 31, 2018 •

edited

Loading

Trott commented Nov 4, 2018

Jimbly commented Nov 4, 2018

joyeecheung commented Nov 4, 2018

joyeecheung left a comment

cjihrig left a comment

Jimbly commented Nov 5, 2018

cjihrig left a comment

cjihrig Nov 5, 2018

cjihrig Nov 5, 2018

Jimbly Nov 5, 2018

cjihrig Nov 5, 2018

Jimbly Nov 5, 2018

cjihrig Nov 5, 2018

Jimbly Nov 5, 2018

Jimbly commented Nov 5, 2018

Trott commented Nov 6, 2018

Trott commented Nov 6, 2018

Jimbly commented Nov 6, 2018

Trott commented Nov 6, 2018

Jimbly commented Nov 6, 2018

paulrutter commented Nov 18, 2018

Trott commented Nov 19, 2018

rvagg commented Nov 19, 2018

child_process: allow 'http_parser' monkey patching again #24006

child_process: allow 'http_parser' monkey patching again #24006

Conversation

Jimbly commented Oct 31, 2018 • edited Loading

Checklist

Trott commented Nov 4, 2018

Jimbly commented Nov 4, 2018

joyeecheung commented Nov 4, 2018

joyeecheung left a comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

Jimbly commented Nov 5, 2018

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig Nov 5, 2018

Choose a reason for hiding this comment

cjihrig Nov 5, 2018

Choose a reason for hiding this comment

Jimbly Nov 5, 2018

Choose a reason for hiding this comment

cjihrig Nov 5, 2018

Choose a reason for hiding this comment

Jimbly Nov 5, 2018

Choose a reason for hiding this comment

cjihrig Nov 5, 2018

Choose a reason for hiding this comment

Jimbly Nov 5, 2018

Choose a reason for hiding this comment

Jimbly commented Nov 5, 2018

Trott commented Nov 6, 2018

Trott commented Nov 6, 2018

Jimbly commented Nov 6, 2018

Trott commented Nov 6, 2018

Jimbly commented Nov 6, 2018

paulrutter commented Nov 18, 2018

Trott commented Nov 19, 2018

rvagg commented Nov 19, 2018

Jimbly commented Oct 31, 2018 •

edited

Loading