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.

Sign up for GitHub

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

Jump to bottom

Broken on node 10.1.0+ #57

Closed
richardlay opened this issue Jul 23, 2018 · 10 comments
Closed

Broken on node 10.1.0+ #57

richardlay opened this issue Jul 23, 2018 · 10 comments

Comments

@richardlay
Copy link

Our tests fail with HPE_INVALID_METHOD when we upgrade to node 10.1.0. Node 10.0.0 still works.
I believe this is related to this change: nodejs/node#20250
@lakeskysea
Copy link

Encountering the same issue and Node upgrade is blocked. Any updates?

@richardlay
Copy link
Author

Not from me. I removed the library and haven't had any response related issues (yet). We're using Node 10.9.0.

@Jimbly
Copy link
Collaborator

Jimbly commented Sep 21, 2018

I've not had time to look at it - dealing with a PS4 launch, so open source stuff has taken a back burner for a while, but maybe my schedule will open up in a couple weeks ^_^. If anyone wants to take a stab at a fix and PR, I'll happily look at it.

@Jimbly
Copy link
Collaborator

Jimbly commented Sep 27, 2018

What exactly is the case that is failing? The automated test suite is (mostly) passing on Node 10, except for some edge cases (e.g. calling req.end() a second time behaves differently now, but no one should be doing that anyway). Nothing I tried fails with the error you've mentioned. Are you using this library as a replacement for http parsing as a client or a server? Can you provide a minimal test case that fails?

@paulrutter
Copy link
Contributor

I upgraded to Node.js 10.12.0, but haven't encountered this specific issue.
I did find a more fundamental issue, which has been fixed in my PR (#58). Can you try my fork to see if it still an issue?

@Jimbly Jimbly mentioned this issue Oct 21, 2018
Closed
@blackknifes
Copy link

blackknifes commented Oct 21, 2018 •
edited
Loading

@Jimbly
I write a test code.
https://github.com/blackknifes/http-parser-test
You can pull this repo.

run server: npm run server
run http-parser-test: npm run dev

The source code use electron V3.x. You can see error on webpage or console of devtools.

@Jimbly
Copy link
Collaborator

Jimbly commented Oct 22, 2018

When I run dev, electron just silently fails, not sure what's up with that. However, looking at the code, it does look like electron is running everything in a child process, so it's quite likely it's the same issue with Node child processes being unable to monkey-patch in HTTP Parser.

@blackknifes
Copy link

When I run dev, electron just silently fails, not sure what's up with that. However, looking at the code, it does look like electron is running everything in a child process, so it's quite likely it's the same issue with Node child processes being unable to monkey-patch in HTTP Parser.

Can fix it at node 10.x ? if can't, I will write http module with socket...

@Jimbly
Copy link
Collaborator

Jimbly commented Oct 26, 2018

Assuming it's the child process issue, there's currently no way to fix it on our end, a fix would need to be made in Node to allow monkey-patching again. v10.0.0 works, but not v10.1.0+. We have an issue opened with the Node.js repo, but have not had any response yet.

Jimbly added a commit to Jimbly/node that referenced this issue Oct 31, 2018
@Jimbly
child_process: allow 'http_parser' monkey patching again
482e23f 
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
@Jimbly Jimbly mentioned this issue Oct 31, 2018
Closed
3 tasks
Trott pushed a commit to Trott/io.js that referenced this issue Nov 8, 2018
@Jimbly @Trott
child_process: allow 'http_parser' monkey patching again
7d86a53 
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>
BridgeAR pushed a commit to nodejs/node that referenced this issue Nov 14, 2018
@Jimbly @BridgeAR
child_process: allow 'http_parser' monkey patching again
a1c7c19 
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 issue Nov 15, 2018
@Jimbly @kiyomizumia
child_process: allow 'http_parser' monkey patching again
395bb86 
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>
codebytere pushed a commit to nodejs/node that referenced this issue Dec 13, 2018
@Jimbly @codebytere
child_process: allow 'http_parser' monkey patching again
abf575d 
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 to nodejs/node that referenced this issue Dec 26, 2018
@Jimbly @MylesBorins
child_process: allow 'http_parser' monkey patching again
04f8d6b 
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>
@Jimbly
Copy link
Collaborator

Jimbly commented Feb 13, 2019

Node v10.15.1 has been released, which includes my PR, which should resolve issues with being unable to monkeypatch in http-parser-js.

There is no way to reliably monkeypatch the http parser in Node versions v10.1 through v10.15.0, so you'll need a newer (or older) Node version. If anyone is still having issues on Node v10.15.1+, please let us know!

@Jimbly Jimbly closed this as completed Feb 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@Jimbly @lakeskysea @richardlay @blackknifes @paulrutter