node 8.12 net,http module emit `connection` bug. TypeError: ParserIncomingMessage is not a constructor .

[whtiehack]

• Version: v8.12.0
• Platform: linux ,windows
• Subsystem:

Here is the code that can be reproduced:

const http = require('http');
const net = require('net');


const httpserver = http.createServer((req,res)=>{
    console.log('~~ http request');
    res.end('aaa');
})

httpserver.on('connection',(socket)=>{
    console.log('httpsever connection');
})

const netserver = net.createServer((socket)=>{
    console.log('netserver connection');
    httpserver.emit('connection',socket);
    // This listener is not important.
    socket.once('data',(data)=>{
        console.log('netserver data',data.toString());
        // This line of code is not important.
        socket.emit('data',data);
    });
}).listen(30332);

The same code run in v10.9.0 and v8.10.0 has been tested without issue.

This is a bug ,because of http_event_connection.

Note: This event can also be explicitly emitted by users to inject connections into the HTTP server. In that case, any Duplex stream can be passed.

[whtiehack]

Temporary solution socket.server = null;

const netserver = net.createServer((socket)=>{
    console.log('netserver connection');
    socket.server = null;
    httpserver.emit('connection',socket);

}).listen(30332);

[tonyjansan]

[2018-09-15 13:10:24.637] [ERROR] console - Caught exception: TypeError: ParserIncomingMessage is not a constructor
at HTTPParser.parserOnHeadersComplete (_http_common.js:81:21)
at socketOnData (_http_server.js:470:20)
at emitOne (events.js:116:13)
at Socket.emit (events.js:211:7)

like some problem, seems critical issue!!!

[AyushG3112]

cc @nodejs/http

Offending line:

https://github.com/nodejs/node/blob/master/lib/_http_common.js#L86

Looks like it should be

const incoming = parser.incoming = ParserIncomingMessage

But I'm not 100% sure

[apapirovski]

The problem is simply one of backporting https://github.com/nodejs/node/pull/20029 to v8.x. I'll take care of it later today if no one gets there first. (Surprised that one slipped by.)

[alexjeffburke]

Hi, sorry to nudge but is there any progress on getting a fix in - I assume that https://github.com/nodejs/node/issues/22880 is the one to watch? Since this was a regression introduced in the active LTS release, are there plans to do an 8.12.1 release? Thanks.

[lpinca]

cc: @nodejs/lts

[MylesBorins]

We don't currently have a timeline for the next 8.x release but it could be escalated if this problem is serious enough. We could potentially get a smaller release out in the next 2 - 3 unless this is deemed to be a severe bug, in which case we could escalate a hotfix

[papandreou]

2 - 3 days or weeks? :)

[MylesBorins]

sorry for missing a word

We generally do a 2 week r.c. process for new LTS releases, and we have not yet backported stuff. So unless this is deemed a severe bug which would escalate a release we wouldn't see a release for at least 2 - 3 weeks.

[whtiehack]

@MylesBorins

This problem is not a serious problem, just using the latest version of node, many libraries can not be used. For example pomelo, pinus and mitm. Therefore, using these libraries requires a rollback of the node version.

---

This problem has been repeated many times before and after, I think we should look for reasons to avoid recurring in the future.

Thanks.

[MylesBorins]

@whtiehack we can add them to CITGM so we don't hit this edge case again

[dougwilson]

FWIW this is a documented feature:

https://nodejs.org/dist/latest-v8.x/docs/api/http.html#http_event_connection

Note: This event can also be explicitly emitted by users to inject connections into the HTTP server. In that case, any Duplex stream can be passed.

It would seem like the Node.js test suite itself should catch the issue instead of relying on CITGM to catch regressions in documented features.

Not saying that it's not worth adding to CIGTM, but maybe someone wants to contribute a test directly.

[lpinca]

Fixed by https://github.com/nodejs/node/commit/e3bddeec18. Closing.