Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: socket hang up when uploading and streaming with pipeline #32184

Closed
rtbo opened this issue Mar 10, 2020 · 11 comments
Closed

Regression: socket hang up when uploading and streaming with pipeline #32184

rtbo opened this issue Mar 10, 2020 · 11 comments
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@rtbo
Copy link

rtbo commented Mar 10, 2020

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 rtbo changed the title Regression: socket hang up when uploading with custom headers Regression: socket hang up when uploading and streaming with pipeline Mar 10, 2020
@rtbo
Copy link
Author

rtbo commented Mar 10, 2020

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
Copy link
Author

rtbo commented Mar 10, 2020

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 addaleax added the stream Issues and PRs related to the stream subsystem. label Mar 11, 2020
@addaleax
Copy link
Member

Bisecting points to 8a2b62e

/cc @ronag

@ronag
Copy link
Member

ronag commented Mar 11, 2020

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.

@ronag

This comment has been minimized.

ronag added a commit to nxtedition/node that referenced this issue Mar 11, 2020
http1 objects are coupled with their corresponding
res/req and cannot be treated independently as
normal streams. Add a special exception for this
in the pipeline cleanup.

Fixes: nodejs#32184
@szmarczak
Copy link
Member

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

@ronag
Copy link
Member

ronag commented Mar 11, 2020

@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
Copy link
Member

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

@ronag
Copy link
Member

ronag commented Mar 11, 2020

#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
Copy link
Member

ronag commented Mar 11, 2020

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
Copy link
Member

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

ronag added a commit to nxtedition/node that referenced this issue Mar 11, 2020
http1 objects are coupled with their corresponding
res/req and cannot be treated independently as
normal streams. Add a special exception for this
in the pipeline cleanup.

Fixes: nodejs#32184

Backport-PR-URL: nodejs#32212
MylesBorins pushed a commit that referenced this issue Mar 12, 2020
http1 objects are coupled with their corresponding
res/req and cannot be treated independently as
normal streams. Add a special exception for this
in the pipeline cleanup.

Fixes: #32184

Backport-PR-URL: #32212
PR-URL: #32197
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants