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

eos and pipeline will deadlock if streams are not in assumed state #28650

Closed
ronag opened this issue Jul 12, 2019 · 4 comments
Closed

eos and pipeline will deadlock if streams are not in assumed state #28650

ronag opened this issue Jul 12, 2019 · 4 comments
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@ronag
Copy link
Member

ronag commented Jul 12, 2019

Currently, pipeline and eos does not take into account whether the streams are "valid" or not to begin with.

Consider this potentially common example:

await mkdirp(path); // async stuff
assert(req.aborted); // aborted by client
const dst = fs.createWriteStream(path);
pipeline(req, dst, common.mustCall(err => {
  assert(err != null);
}));

or less common

const dst = fs.createWriteStream(path);
await doAsyncStuff();
assert(dst.destroyed);
pipeline(req, dst, common.mustCall(err => {
  assert(err != null);
}));

This will fail... even though I believe it should error with a premature close (or something similar)?

Basically eos and pipeline makes assumptions of the arguments which are not enforced...

@ronag ronag changed the title eos and pipeline will deadlock if source is already closed eos and pipeline will deadlock if streams are not in valid state Jul 12, 2019
@ronag ronag changed the title eos and pipeline will deadlock if streams are not in valid state eos and pipeline will deadlock if streams are not in assumed state Jul 12, 2019
@ronag
Copy link
Member Author

ronag commented Jul 12, 2019

I would suggest that eos be updated to work similarly to on-finished where the callback is invoked if the stream is already in a "complete" state.

@Fishrock123 Fishrock123 added the stream Issues and PRs related to the stream subsystem. label Jul 12, 2019
@ronag
Copy link
Member Author

ronag commented Oct 6, 2019

This was fixed in a PR

@ronag ronag closed this as completed Oct 6, 2019
@jstewmon
Copy link

👋 Hi, @ronag do you happen to remember the PR that was supposed to fix this issue? I think I'm encountering the same issue, for which I've created a minimal reproduction.

const { finished, PassThrough, pipeline } = require('stream');
const http = require('http');

const server = http.createServer((req, res) => {
  console.log('server: request received');
  const body = new PassThrough().end('foo');
  setTimeout(() => {
    console.log('server: pipeline begin');
    return pipeline(body, res, (err) => {
      console.log('server: pipeline end');
      if (err) {
        console.error('server: pipeline err:', err);
      }
    });
  }, 100);
});

server.listen({ host: '127.0.0.1', port: 0 }, (err) => {
  const addr = server.address();
  const uri = `http://${addr.address}:${addr.port}`;

  const req = http.request(uri).end();
  req.on('response', async (res) => {
    console.log('response received');
    for await (const chunk of res) {
      console.log('client: body: ', chunk.toString());
    }
    console.log('closing server');
    server.close(() => {
      console.log('server closed');
    });
  });

  finished(req, (err) => {
    console.log('client: end');
    if (err) {
      console.error('client: error: ', err);
    }
  });

  // aborted request will cause server pipeline to never complete
  setTimeout(() => req.abort(), 50);
});

Without the req.abort() call, the process with exit with output:

client: end
server: request received
server: pipeline begin
server: pipeline end
response received
client: body:  foo
closing server
server closed

With the req.abort() call, the process will not exit with output:

client: end
server: request received
server: pipeline begin

I get the same behavior on node versions 10, 12 and 13.

Is this the expected behavior? If so, can you point me in the right direction on how this should be handled on the server side?

cc @mcollina

@ronag
Copy link
Member Author

ronag commented Apr 14, 2020

#31509

It will be included in Node 14

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

No branches or pull requests

3 participants