Add support for Node 10.12.x.

[paulrutter]

Following https://github.com/nodejs/node/blob/master/src/node_http_parser.cc, the core HTTP parser now validates the arguments given to the constructor. This used to by a string but is now a number (0,1, 2).

This is a short term solution that adds support for Node.js 10.x, but in the long term the following might prove problematic:

• process.binding() is deprecated. Although it works in Node.js 10, it might not work for newer versions. I guess this is more of a fundamental issue; if Node.js wants to disable these kind of monkey patches, there is not much we can do about it.
• HTTP2 support; since this is now core Node.js functionality, i'm not sure how compatible http-parser-js is with this. This fix certainly doesn't add support for this.

[paulrutter]

Added Node.js 8 and 10 to Travis, but found Node.js 10 to contain failing unit tests.
Will try to fix in my branch where possible, but not sure if i'm able to do that right now.

[Jimbly]

I'm confused by this change. Those values have been numbers in previous Node versions too (they are back to at least v6 I checked randomly), but they should be opaque to the Node Javascript code, they're only ever passed to "new HTTPParser()", which is our code. Am I missing something?

[paulrutter]

@Jimbly that took me some thing to figure out as well.
It looks like this has been changed recently (see https://github.com/nodejs/node/blob/eb9748d222061381236f19cbe162cf9eb2e034ad/lib/_http_client.js and https://github.com/nodejs/node/blob/eb9748d222061381236f19cbe162cf9eb2e034ad/lib/_http_server.js); the http client/server code calls "parser.reinitialize" with an enum, which translates to a number. Reinitialize calls the constructur with the number arguments.

[Jimbly]

It looks to me like it's calling parser.reinitialize(HTTPParser.RESPONSE, which isn't a number, that's the string we were previously exporting? Where's the call with a number?

[paulrutter]

@Jimbly

Sorry for the messy explanation, you're right.
The problem lies with "https://github.com/nodejs/node/blob/master/src/node_http_parser.cc#L467". It validates the arguments to the "Reinitialize" call; is must now be a number or an exception is thrown.

The unit test changes now fail 6 and 8, so more work there i guess.

[Jimbly]

Our .REQUEST and .RESPONSE values should never get passed to node_http_parser.cc, that's the module we're replacing. If so, something else is going wrong (maybe related to process binding stuff?).

[Jimbly]

I did notice when I ran the tests on v10 myself that a few of the tests were getting into the native Node parser, which probably shouldn't be happening (otherwise the test is testing the wrong parser, although maybe those particular tests always were), but I didn't investigate what was causing that.

[paulrutter]

Process.binding is deprecated in node.js 10, so the implementation might have changed a bit?
The question is why node_http_parser.cc::Reinitialize is called. Should that happen at all?

Hmm, they now use internalBinding to retrieve the http_parser, which used to be different with process.binding? See https://github.com/nodejs/node/blob/eb9748d222061381236f19cbe162cf9eb2e034ad/lib/_http_client.js#L27.

[Jimbly]

It looks like process.binding still works, and "http_parser" is "whitelisted for access via process.binding()", so I don't think that's an immediate problem, but probably will be in the future. It's possible some node bootstrapping order has changed so we're not getting our monkey patch in before _http_common.js pulls out HTTPParser, or something like that though, if some internal module is requiring _http_common before user code starts running... but in general it seems like our monkey patch is working, just not on a couple tests.

[paulrutter]

Ok i think i understand. I'll close this pr and when i think i have the solution to the issue, open a new PR.

[Jimbly]

Okay, cool, thanks for diving into this!

[paulrutter]

@Jimbly

I dug a little deeper today, and i think i at least found in what situations the monkey patch isn't applied correctly. The test "test-http-server-stale-close.js" ends up throwing native code error in "Reinitialize":

npm[15416]: src\node_http_parser.cc:467: Assertion args[0]->IsInt32()' failed.
1: 00007FF65F96ACB5
2: 00007FF65F9444E6
3: 00007FF65F9445B1
4: 00007FF65F8E12EC
5: 00007FF6601633A2
6: 00007FF66016480D
7: 00007FF660163899
8: 00007FF66016377B
9: 0000014509B5C5C1`

This only seems to happen when child_process.fork is used. When switching over to child_process.spawn, all seems well again. See changed test:

'use strict';
require('../common');
var http = require('http');
var util = require('util');
var spawn = require('child_process').spawn;

if (process.env.NODE_TEST_FORK_PORT) {
  var req = http.request({
    headers: {'Content-Length': '42'},
    method: 'POST',
    host: '127.0.0.1',
    port: +process.env.NODE_TEST_FORK_PORT,
  }, process.exit);

  req.write('BAM');
  req.end();
}
else {
  var server = http.createServer(function(req, res) {
    res.writeHead(200, {'Content-Length': '42'});
    req.pipe(res);
    req.on('close', function() {
      server.close();
      res.end();
    });
  });
  server.listen(0, function() {
    spawn("node", [__filename], {
      env: util._extend(process.env, {NODE_TEST_FORK_PORT: this.address().port})
    });
  });
}

Now the million dollar question; what changed in child_process.fork? Since the main difference of fork vs spawn is about the IPC channel between parent and child, there might be a bootstrapping difference specifically for IPC communication between Node.js 8 and 10.

At BlueConic, we are using http-parser-js in child processes (started with fork) a lot, so that explains why i saw the issue instantly. So, for our usecase, we need to get this fixed.

I will try to dig a little deeper into the difference in fork between Node8 and 10, to see if we can isolate it.

[paulrutter]

What stands out is that Node.js child_process in master now requires http_parser and _http_common, where this wasn't the case in Node.js 8.x.

Not sure yet why spawn mitigates the issue.
Probably the bootstrapping code for the process being forked is a bit different than when using spawn, but i haven't found the code that causes this behavior. @Jimbly any ideas?

[Jimbly]

I should have some time today to poke into this, I'll see if I can find a cause. Nothing is obvious from skimming the code, but it shouldn't be too hard to add some logging to the Node internals and see what's happening. I'm a little worried that: if the first line of user code in our module is overriding the process binding, and that's not good enough, there might be nothing that can be done, short of requesting some upstream changes to Node.js - and supporting modules that override Node.js internals is very much something they don't want to be concerned with, but if it's a generalized fix, not just exposing something for us, then maybe they'd take it ^_^.

[Jimbly]

Well, I'm not sure I'm closer to a solution or not, but I've tracked down a lot of details. The specific minor version where this started to fail was between v10.0.0 and v10.1.0, and, as you suspected, that's where the additional require() from internal/child_process of http_parser and _http_common happen (most importantly the latter, since then _http_common caches a HTTPParser constructor pre-monkeypatch).

I've been doing my testing on the v10.1.0 code, as it just gives a JS-level error instead of a process crash, which makes things much easier to debug.

The actual include path/stack for how _http_common is getting loaded at startup is:

• _http_common.js
• internal/child_process.js:35:24
• child_process.js:42:23
• Object.setupChannel (internal/process.js:246:5)

The reason this only happened for you with fork() and not spawn() is that Object.setupChannel() is only called if the child process has an IPC with the parent. Calling spawn() with stdio: ['inherit', 'inherit', 'inherit', 'ipc'] has the same effect.

The problem that causes crashes is that then, later, _http_client/server get loaded, after our monkey-patching, so they're using our HTTParser.* values and passing them on into a non-monkey-patched native parser object.

I can't think of any obvious solution to this, either in our code or a general-purpose patch to Node. We might need to do the monkey-patching differently, assuming callers already have an instance of the native HTTPParser constructor, but I do not know of any way to change a function/ctor's code retroactively (seems that it might not be possible), however we can modify it's prototype, so we could maybe allow the default/native constructor to be called and just override all of the functions with some lazy construction... but that's bound to be messy and we never get access to the original parameters to the constructor, so I'm not particularly hopeful that would work... I'm out of time for the day, just wanted to share what I've found so far.

Also, maybe we should "re-open" this, not to necessarily merge the code, but to keep our conversation in an open issue.

[paulrutter]

Thanks for your investigation. I'll think about some more as well.

[Jimbly]

I've done some more thinking and posted an issue in the Node.js repo, will hopefully get some direction or advice from them. I think it might not be too bad to fix Node to allow this to work for now, but probably nothing guaranteed future-safe... but this module breaks on roughly every-other major Node version, so that's nothing new ;)

[paulrutter]

This issue will be fixed with a new Node.js release: nodejs/node#24006.
Lets wait on the new release and close this PR if Node.js 10.x is working correctly then.
There might be some unit tests which need to be changed.

[paulrutter]

Now Node.js 10.15.1 is released (in which #24006 is fixed), i'll work to make this branch run on Node.js 10 again.

[paulrutter]

@Jimbly I've reverted all of the changes in http-parser.js, but still some unit tests failed.
I've fixed most of them, but still two errors remain:

=== release test-http-blank-header ===
Path: parallel/test-http-blank-header
Command: node /home/travis/build/creationix/http-parser-js/tests/parallel/test-http-blank-header.js
--- TIMEOUT ---
=== release test-http-upgrade-server ===
Path: parallel/test-http-upgrade-server
Command: node /home/travis/build/creationix/http-parser-js/tests/parallel/test-http-upgrade-server.js
--- TIMEOUT ---

It looks like Node.js 10 still does some things different.