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

Is monkey-patching still working? #65

Closed
disfated opened this issue Oct 22, 2019 · 3 comments · Fixed by #67
Closed

Is monkey-patching still working? #65

disfated opened this issue Oct 22, 2019 · 3 comments · Fixed by #67

Comments

@disfated
Copy link

On node v12.13 having a hard time to make use of http-parser-js.

This trick

process.binding('http_parser').HTTPParser = require('http-parser-js').HTTPParser;

seems not working any more.

Is anyone having success with it on actual node versions or I'm missing something?

@Jimbly
Copy link
Collaborator

Jimbly commented Oct 22, 2019

I did some digging, and there's a bunch of things going wrong...

  • Node's parser binding in v12 is now called 'http_parser_llhttp', however they did not add that to the whitelist of modules exposed through process.binding, so there's no way to override it. v13 renames it back to http_parser, so that's good, at least.
  • Luckily, you can tell node v12 to use the older parser binding with node --http-parser=legacy, then it's called 'http_parser' again, which is monkey-patchable
  • Unfortunately, they changed the parser API again, in a way that http-parser-js does not handle (looks pretty minor, just changed it to empty constructor plus an .initialize() call instead of passing things in the constructor), so even with --http-parser=legacy, this module is not working
    • PRs welcome to fix this! I think that'll also fix it on node v13+, but v12 will probably require --http-parser=legacy in order to monkey patch.

This was referenced Oct 27, 2019
@Jimbly Jimbly closed this as completed in #67 Nov 2, 2019
@Jimbly
Copy link
Collaborator

Jimbly commented Nov 2, 2019

I've fixed http-parser-js to work on Node v12.x and v13.0.1. However, on Node v12, you must launch Node with node --http-parser=legacy in order to monkey-patch. Should work after doing that, though, all tests are passing! Published to NPM as v0.5.2.

@Jimbly Jimbly reopened this Nov 2, 2019
Jimbly added a commit to Jimbly/node that referenced this issue Nov 2, 2019
Fixes inability to monkey-patch the HTTP Parser on Node v12.x.
This is not an issue on Node v13+ since the parser was renamed
back to the old (already monkey-patchable) `http_parser` name,
however the test in master could also use updating.

Fixes: creationix/http-parser-js#65
MylesBorins pushed a commit to nodejs/node that referenced this issue Jan 8, 2020
Fixes inability to monkey-patch the HTTP Parser on Node v12.x.
This is not an issue on Node v13+ since the parser was renamed
back to the old (already monkey-patchable) `http_parser` name,
however the test in master could also use updating.

Fixes: creationix/http-parser-js#65

PR-URL: #30222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BethGriggs pushed a commit to nodejs/node that referenced this issue Feb 6, 2020
Fixes inability to monkey-patch the HTTP Parser on Node v12.x.
This is not an issue on Node v13+ since the parser was renamed
back to the old (already monkey-patchable) `http_parser` name,
however the test in master could also use updating.

Fixes: creationix/http-parser-js#65

PR-URL: #30222
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@Jimbly
Copy link
Collaborator

Jimbly commented Jan 2, 2021

Monkey-patching is working as of all current Node versions (has been for quite a while, just closing this issue now). For reference, checked yesterday on

  • v6.9.1
  • v8.11.3
  • v10.16.0
  • v12.20.0 (requires --http-parser=legacy command line option)
  • v13.11.0
  • v14.9.0
  • v15.5.0

@Jimbly Jimbly closed this as completed Jan 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants