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

Stream destruction breaks async-iteration and ends nodejs event loop #23730

Closed
mika-fischer opened this issue Oct 18, 2018 · 5 comments
Closed
Assignees
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.

Comments

@mika-fischer
Copy link
Contributor

mika-fischer commented Oct 18, 2018

  • Version: v10.12.0
  • Platform: Windows 10 64-bits
  • Subsystem: streams

Async iteration over a readable stream breaks horribly if for any reason the stream is destroyed while the iteration is still running (or even if it is destroyed before starting the iteration). I would expect that the iteration runs normally or throws an exception. What happens instead is that the whole event loop is shut down.

Most notably this makes async iteration useless with pipeline.

Test code:

const {createReadStream} = require('fs');
const {pipeline} = require('stream');
const {createDeflate} = require('zlib');

async function iterate(name, stream, destroy = false) {
    try {
        console.log(`${name}: Starting loop`);
        for await (const chunk of stream) {
            console.log(`${name}: Got a chunk`);
            if (destroy) {
                console.log(`${name}: destroying stream`);
                stream.destroy();
            }
        }
        console.log(`${name}: loop ended`);
    } catch (err) {
        console.log(`${name}: loop ended with error: ${err}`);
    }
}

async function main() {
    // These work fine
    await iterate('oneStream', createReadStream('test.js'));
    await iterate('oneStreamError', createReadStream('doesnotexist'));

    // Any of these just shuts down the node event loop...
    await iterate('oneStreamDestroy', createReadStream('test.js'), true);
    await iterate('pipeline', pipeline(createReadStream('doesnotexist'), createDeflate(), err => console.log(`pipeline ended: ${err}`)));
    const stream = createReadStream('test.js');
    stream.destroy();
    await iterate('destroyedStream', stream);
}

main().catch(console.error);

Output:

> node test.js
oneStream: Starting loop
(node:21664) ExperimentalWarning: Readable[Symbol.asyncIterator] is an experimental feature. This feature could change at any time
oneStream: Got a chunk
oneStream: loop ended
oneStreamError: Starting loop
oneStreamError: loop ended with error: Error: ENOENT: no such file or directory, open 'C:\Users\mfischer\src\tests\node-stream-async-iteration\doesnotexist'
oneStreamDestroy: Starting loop
oneStreamDestroy: Got a chunk
oneStreamDestroy: destroying stream
@targos targos added the stream Issues and PRs related to the stream subsystem. label Oct 18, 2018
@targos
Copy link
Member

targos commented Oct 18, 2018

@nodejs/streams

@Hakerh400
Copy link
Contributor

Simplified repro:

(async () => {
  var stream = require('fs').createReadStream(__filename);
  stream.destroy();
  await stream[Symbol.asyncIterator]().next();
  console.log('Done');
})().catch(console.log);

The resolve function assigned to iterator[kLastResolve] never gets called:

const data = iterator[kStream].read();
if (data) {
iterator[kLastPromise] = null;
iterator[kLastResolve] = null;
iterator[kLastReject] = null;
resolve(createIterResult(data, false));
} else {
iterator[kLastResolve] = resolve;
iterator[kLastReject] = reject;
}

@mcollina mcollina self-assigned this Oct 18, 2018
@mcollina mcollina added the confirmed-bug Issues with confirmed bugs. label Oct 18, 2018
@mcollina
Copy link
Member

Definitely it's a bug, as @Hakerh400 showed.

@mika-fischer the event loop is shutting down because there is nothing asynchronous left to do. If you add the following:

setInterval(() => {
  console.log('a second passed')
}, 1000)

The application will keep running

@mcollina
Copy link
Member

See #23785 for a fix.

mcollina added a commit to mcollina/node that referenced this issue Oct 24, 2018
@mcollina
Copy link
Member

Fixed in 3ec8cec.

mcollina added a commit that referenced this issue Oct 24, 2018
Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
targos pushed a commit that referenced this issue Oct 24, 2018
Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
MylesBorins pushed a commit that referenced this issue Nov 26, 2018
Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
rvagg pushed a commit that referenced this issue Nov 28, 2018
Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
MylesBorins pushed a commit that referenced this issue Nov 29, 2018
Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants