Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

Jimbly
Copy link
Contributor

@Jimbly Jimbly commented 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

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-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Oct 31, 2018
@Trott
Copy link
Member

Trott commented Nov 4, 2018

@nodejs/child_process This could use some reviews.

@Jimbly
Copy link
Contributor Author

Jimbly commented 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
Copy link
Member

Copy link
Member

@joyeecheung joyeecheung left a 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

Copy link
Contributor

@cjihrig cjihrig left a 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.

@Jimbly
Copy link
Contributor Author

Jimbly commented Nov 5, 2018

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

Copy link
Contributor

@cjihrig cjihrig left a 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);
Copy link
Contributor

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)
Copy link
Contributor

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 😄

Copy link
Contributor Author

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;
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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');

Copy link
Contributor Author

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.

@Jimbly
Copy link
Contributor Author

Jimbly commented Nov 5, 2018

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

@Trott
Copy link
Member

Trott commented Nov 6, 2018

@Trott 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
@Trott
Copy link
Member

Trott commented 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
Copy link
Contributor Author

Jimbly commented Nov 6, 2018

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

@Trott
Copy link
Member

Trott commented 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
Copy link
Contributor Author

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

BridgeAR pushed a commit that referenced this pull request Nov 14, 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

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>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 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: 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
Copy link

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

@Trott
Copy link
Member

Trott commented 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
Copy link
Member

rvagg commented Nov 19, 2018

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

codebytere pushed a commit that referenced this pull request Dec 13, 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

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>
MylesBorins pushed a commit that referenced this pull request Dec 26, 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

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>
@codebytere codebytere mentioned this pull request Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants