-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
child_process: allow 'http_parser' monkey patching again #24006
Conversation
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
@nodejs/child_process This could use some reviews. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless of the monkey-patchablility, it makes sense to lazy load the HTTP parser in child_process, so LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@cjihrig I've added a test (which fails on Node v10.1.0+, passes with this fix). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM with a couple optional comments.
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also assert on signal
being null
.
|
||
// Test in our process | ||
doTest(); | ||
// Test in a child process with IPC (specific case of https://github.com/nodejs/node/issues/23716) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 =).
} | ||
} | ||
DummyParser.REQUEST = Symbol(); | ||
process.binding('http_parser').HTTPParser = DummyParser; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and I think this should be using internalBinding
, since we recently moved toward gradually deprecating all of process.binding()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Updated test case code after comments from @cjihrig. I think is is now ready to go! |
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? |
Does the the fork() call need |
I think you can run |
Looks like it reproduced with running |
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>
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 |
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>
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: #23716
Fixes: creationix/http-parser-js#57
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes