Repository navigation
Fails with node.js 9.6.0+ #48
Description
Everything works fine with 9.5.0 and below, but on 9.6.0 a bunch of the tests fail with:
1) Mitm via Http.request as request must emit request on Mitm:
Uncaught TypeError: ParserIncomingMessage is not a constructor
at HTTPParser.parserOnHeadersComplete (_http_common.js:81:21)
at socketOnData (_http_server.js:472:20)
at Socket.emit (events.js:125:13)
at addChunk (_stream_readable.js:269:12)
at readableAddChunk (_stream_readable.js:256:11)
at Socket.Readable.push (_stream_readable.js:213:10)
at InternalSocket.onread (net.js:590:20)
at InternalSocket.readData (/home/andreas/work/node-mitm/lib/internal_socket.js:109:13)
at InternalSocket.emit (events.js:125:13)
at addChunk (_stream_readable.js:269:12)
at readableAddChunk (_stream_readable.js:256:11)
at InternalSocket.Readable.push (_stream_readable.js:213:10)
at /home/andreas/work/node-mitm/lib/internal_socket.js:57:40
at process._tickCallback (internal/process/next_tick.js:150:11)
Activity
papandreou commentedon Feb 24, 2018
Seems like this commit is the culprit: nodejs/node@c247cb02a1
Wire up internals for node.js 9.6.0+ compatibility
fyhao commentedon Mar 5, 2018
Yes, thanks, I encountered the same issue here. May I know when will this issue fixed and merged into master?
papandreou commentedon Mar 5, 2018
@fyhao, I've fixed it in #49 and published my mitm-papandreou fork. You can switch to that if you're in a hurry.
dbellizzi commentedon Sep 11, 2018
This happens in node 8.12.0 as well, which was released yesterday.
shudingbo commentedon Sep 12, 2018
me too,when upgrade 8.12.0
papandreou commentedon Sep 12, 2018
While you're waiting, you can switch to the
mitm-papandreoupackage, which includes this fix.moll commentedon Sep 12, 2018
On it! Checking out @papandreou's work and ensuring it works on v10 as well. Hope to get a fix out tomorrow.
moll commentedon Sep 13, 2018
Thanks again, @papandreou, and others, for the Node v9.6+ debug help. While I was checking that out, I went ahead and fixed Node v10 and v8.12 as well as they seemed to introduce a few other incompatibilities.
I'll cut a release once I've heard some of you confirm that the master branch works on your machine with a real project. Doing the same myself.
moll commentedon Sep 16, 2018
I'll publish the new version tomorrow. It seemed to continue to work fine on my own calendar app and presumably will on all of yours, too. :)
papandreou commentedon Sep 16, 2018
I've tested it a bit, and it does seem to fix the node.js 10 problems 🎉
I ran into some weirdness with node 8.12.0, though: https://travis-ci.org/assetgraph/assetgraph/jobs/428921736#L2808-L2821 -- but it seems to be due to a bad backport that has a fix on the way.
No matter what it's not a regression in node-mitm, so I say
moll commentedon Sep 16, 2018
That failing test seems to be using your fork which doesn't have the latest changes from here, right? I noticed the
serverissue and renamed that property to work-around that v8.12 prob.moll commentedon Sep 16, 2018
While it may not have been obvious, Mitm's
serverproperty on the client side socket was unrelated to how Node v8.12 started using it internally. ^_^papandreou commentedon Sep 16, 2018
The failing test was running mitm-papandreou@1.3.3-patch5, which does include the latest changes here. I merged in master: papandreou@02f74f7
... But let's hope it sorts itself out when they fix node 8.12
moll commentedon Sep 17, 2018
Gotcha. Do unexpected-mitm and httpception emit events again to trigger that bug on Node v8.12? Mitm.js's own triggering of
connectiondoes work now.Anyways, I published v1.4 with the Node v8.12, v9 and v10 fixes so people who don't follow Mitm.js's master branch can continue using it. Thanks again everyone!
papandreou commentedon Sep 17, 2018
Thanks a lot for sorting it all out!
No. There's a few places where the unexpected-mitm emits an
errorevent on the client socket to simulate errors, but none of that code is in play for the test that failed.I'll pick it up again and look for the root cause if the next node 8.x release still triggers it.
moll commentedon Sep 17, 2018
Okei. Just FYI, my own app's tests that use Express.js with Mitm.js don't trigger errors under v8.12.
papandreou commentedon Sep 17, 2018
Yeah, there's a good chance that it's a bug in unexpected-mitm, or a weird edge case.