Regression: socket hang up when uploading and streaming with pipeline

[rtbo]

• Version: v13.10.1 (and v13.10.0)
• Platform: Linux laptop-remi 5.3.0-40-generic docs: replace all instances of node.js with io.js because trademark. #32-Ubuntu SMP Fri Jan 31 20:24:34 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

What steps will reproduce the bug?

git clone git@github.com:rtbo/node-failed-upload
cd node-failed-upload
npm install
npm run test

How often does it reproduce? Is there a required condition?

Everytime

What is the expected behavior?

With node v13.9.0:

> NODE_ENV=test mocha

  Upload test
    ✓ should work

  1 passing (30ms)

What do you see instead?

With node v13.10.0 and higher:

> NODE_ENV=test mocha

  Upload test
    1) should work

  0 passing (25ms)
  1 failing

  1) Upload test
       should work:
     Error: socket hang up
      at connResetException (internal/errors.js:613:14)
      at Socket.socketOnEnd (_http_client.js:463:23)
      at endReadableNT (_stream_readable.js:1201:12)
      at processTicksAndRejections (internal/process/task_queues.js:84:21)

[rtbo]

TL;DR;

Client receive "socket hang up" error when reaching this middleware:

const Koa = require("koa");
const fs = require("fs");
const util = require("util");
const stream = require("stream");

const app = new Koa();

const pipeline = util.promisify(stream.pipeline);

app.use(async ctx => {
  const { "content-length": length } = ctx.request.headers;

  const openFlags = fs.constants.O_WRONLY | fs.constants.O_CREAT;

  const ws = fs.createWriteStream("test.bin", {
    flags: openFlags,
    encoding: "binary"
  });

  /*
    !!! commenting the following line disable the "socket hang up" error !!!
   */ 

  await pipeline(ctx.req, ws);

  ctx.status = 200;
  ctx.response.body = {
    length: parseInt(length)
  };
});

module.exports = app;

[rtbo]

Quite hard to debug, because the pipeline call succeeds. The file is actually written, but there seems to be side effect on the http socket.

[addaleax]

Bisecting points to 8a2b62e

/cc @ronag

[ronag]

Yea, there is an unfortunate coupling between the req and res objects... should probably be decoupled when the req object 'finish'es, but that's semver major.

I'll try to prepare 13.x fix later today.

[szmarczak]

@ronag I thought that the IncomingMessage pipeline fix was tagged as a major change (which I strongly disagree with the choice).

[ronag]

@ronag I thought that the IncomingMessage pipeline fix was tagged as a major change (which I strongly disagree with the choice).

Sorry, I'm confused? You link to two different PR's there.

[szmarczak]

This solution does the same as #32153 because you don't call the destroyer if the stream is an IncomingMessage.

[ronag]

#32153 will set socket = null which is a potentially breaking change for code that assumes that socket is not null. They do not do the same thing (even if they solve the same problem for you).

[ronag]

I was just working on backporting the other one #32212 (actually the same minute you commented). Maybe that's why I got confused by your comment.

[szmarczak]

Ok, I get it now, thanks. Keep up the good work!