Master fails on CI on node 10

[gustavnikolaj]

The last time a change on master landed it passed on both node 10 and 8. I noticed that a few of my PRs started failing on node 10 - even when I made no changes to the actual code, and only to the supporting environment.

I restarted the latest node 10 build of master, and that fails too: https://travis-ci.org/papandreou/express-processimage/jobs/482325435

Maybe it's a regression in a newer version of node 10, or a caret-dependency that introduced a regression.

  128 passing (12s)
  2 pending
  1 failing
  1) express-processimage
       against a real server
         should destroy the created filters when the client closes the connection prematurely:
     UnexpectedError: 
expected destroy was called once
      at expect.promise.then (test/processImage.js:1394:11)
      at Array.forEach (<anonymous>)
      set UNEXPECTED_FULL_TRACE=true to see the full stack trace

I'll see if I can get it working before I start the actual work I have to do... ;-)

[gustavnikolaj]

It's a change in node core.

The test passes on node v10.15.0 but fails on v10.15.1 and v10.15.2 (latest).

[gustavnikolaj]

nodejs/node@v10.15.0...v10.15.1

[gustavnikolaj]

These two commits look suspicious:

• nodejs/node@10a2727
• nodejs/node@897114b

Only one of the two removed paths that call into this.destroy is reintroduced in the "revert" commit.

[papandreou]

Thanks for doing the detective work! 🕵️

I haven't looked into it, but I guess this indicates that we might leak memory in 10.15.1 when a client closes the connection prematurely?

[gustavnikolaj]

I've dug deeper into the rabbit hole and found that both node.js 8.0.0 and a later minor on 8 introduces changes that break hijackresponse. To rule out that this is a fallout of that I have to start out fixing hijackresponse. Unfortunately, the conclusion thus far is that we have to assume that hijackresponse is not working properly in anything but node.js 6 and 7.

[gustavnikolaj]

I have some good / bad news... After spending some time with hijackresponse I realized that it was actually due to some of the mocked out response and requests objects I had used in some of the tests. When I rewrote the tests in hijackresponse to a real server instead, I don't see any changes in behavior on node 8 or 10. node 11 fails though, so I made a separate PR which documents that, which I'll pick back up later. gustavnikolaj/hijackresponse#16

[gustavnikolaj]

I released a new major version (4.0.0) as I dropped node 4 support, and added node 8 and 10 to the travis ci build. It has no functional changes though.