search

moll / node-mitm

Intercept and mock outgoing Node.js network TCP connections and HTTP requests for testing. Intercepts and gives you a Net.Socket, Http.IncomingMessage and Http.ServerResponse to test and respond with. Super useful when testing code that hits remote servers.
Other
640 stars 48 forks source link

Fails with node.js 9.6.0+ #48

Closed papandreou closed 6 years ago

papandreou commented 6 years ago

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)
papandreou commented 6 years ago

Seems like this commit is the culprit: https://github.com/nodejs/node/commit/c247cb02a1

fyhao commented 6 years ago

Yes, thanks, I encountered the same issue here. May I know when will this issue fixed and merged into master?

papandreou commented 6 years ago

@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 commented 6 years ago

This happens in node 8.12.0 as well, which was released yesterday.

shudingbo commented 6 years ago

This happens in node 8.12.0 as well, which was released yesterday.

me too,when upgrade 8.12.0

papandreou commented 6 years ago

While you're waiting, you can switch to the mitm-papandreou package, which includes this fix.

moll commented 6 years ago

On it! Checking out @papandreou's work and ensuring it works on v10 as well. Hope to get a fix out tomorrow.

moll commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

I've tested it a bit, and it does seem to fix the node.js 10 problems :tada:

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 :shipit:

moll commented 6 years ago

That failing test seems to be using your fork which doesn't have the latest changes from here, right? I noticed the server issue and renamed that property to work-around that v8.12 prob.

moll commented 6 years ago

While it may not have been obvious, Mitm's server property on the client side socket was unrelated to how Node v8.12 started using it internally. ^_^

papandreou commented 6 years ago

That failing test seems to be using your fork which doesn't have the latest changes from here, right? I noticed the server issue and renamed that property to work-around that v8.12 prob.

The failing test was running mitm-papandreou@1.3.3-patch5, which does include the latest changes here. I merged in master: https://github.com/papandreou/node-mitm/commit/02f74f7aa9eecc67a1ea2ed370e36fb8a3063562

... But let's hope it sorts itself out when they fix node 8.12

moll commented 6 years ago

Gotcha. Do unexpected-mitm and httpception emit events again to trigger that bug on Node v8.12? Mitm.js's own triggering of connection does 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 commented 6 years ago

Thanks a lot for sorting it all out!

Gotcha. Do unexpected-mitm and httpception emit events again to trigger that bug on Node v8.12?

No. There's a few places where the unexpected-mitm emits an error event 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 commented 6 years ago

Okei. Just FYI, my own app's tests that use Express.js with Mitm.js don't trigger errors under v8.12.

papandreou commented 6 years ago

Yeah, there's a good chance that it's a bug in unexpected-mitm, or a weird edge case.