nodejs / node

Node.js JavaScript runtime βœ¨πŸ’πŸš€βœ¨
https://nodejs.org
Other
106.66k stars 29.09k forks source link

readable[Symbol.asyncIterator]().next() on stream causes immediate exit (no error) #34219

Open kwkelly opened 4 years ago

kwkelly commented 4 years ago

What steps will reproduce the bug?

When next() is called in the program below, nothing else gets run. list.txt and list2.txt are just two line files line1\nline2 and line3\nline4

This repl.it shows the issue. https://repl.it/repls/FrightenedPastLogin#index.js

const fs = require("fs");
const readline = require("readline");

async function main() {
  const linereader = readline.createInterface({
    input: fs.createReadStream("./list.txt"),
  });

  for await (const s of fs.createReadStream("./list2.txt")){}
  console.log(await linereader[Symbol.asyncIterator]().next())

  // nothing below here gets run
  console.log("=============");
  for await (let s of linereader) {
    console.log(s);
  }
  console.log("=============");

  return "test";
}

main()
  .then((t) => console.log(t))
  .catch((e) => console.log(e));

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

Always

What is the expected behavior?

The rest of the program should execute. The following should print

node test.js
{ value: 'line1', done: false }
=============
line2
=============
test

What do you see instead?

Nothing:

node test.js

Possibly related to #33792 #34035?

devsnek commented 4 years ago

I believe this is a duplicate of https://github.com/nodejs/node/issues/33463

kwkelly commented 4 years ago

@devsnek it may be a duplicate and probably related, but the behavior is slightly different since "node simply exits if it has nothing else to do" as in #33792

devsnek commented 4 years ago

@kwkelly that's just because your code is waiting for a promise that will never be resolved. if you want, try replacing that line with await new Promise(() => {}).

kwkelly commented 4 years ago

Ok, I think I understand now. So next() is returning a promise that never fulfills. But it should always return a promise that resolves to an IteratorResult object (which may indicate done).

devsnek commented 4 years ago

@kwkelly but since it wraps an event emitter it is technically never done, it's waiting for the next 'line' event that will never come. @nodejs/readline maybe we should remove readline's current async iterator and add a stream version of readline instead.

ronag commented 4 years ago

add a stream version of readline instead.

A stream can also be consumed as an async iterator πŸ˜„

devsnek commented 4 years ago

@ronag yeah that's the idea. streams will exhibit backpressure so this dropping events thing won't be a problem.

jfriend00 commented 4 years ago

The overall problem here is that readline.createInterface() immediately adds a data event listener to the stream and resumes the stream. So, if you don't synchronously add a line event listener or synchronously create the async iterator, then the readline interface will happily get data events and fire off line events, but nobody will be listening to them and they will be lost. This means that readline.createInterface() is not really compatible with other asynchronous code in many ways. The overall solution here is for the readline interface to NOT install the data listener and not resume the stream until someone installs a line listener. That would prevent loss of data when events are occurring, but nobody is listening to them.

Some other discussion of this issue here: https://stackoverflow.com/questions/62885667/why-does-this-readline-async-iterator-not-work-properly#comment111206701_62885667

devsnek commented 4 years ago

we should just expose a stream version of it.

jfriend00 commented 4 years ago

@devsnek - Yes, readline is a mess for this type of use (numerous bugs). A nice clean stream version would be great.