readline: add support for async iteration #18904

prog1dev · 2018-02-21T16:36:56Z

Sync readline API with for-await-of support in readable streams

Resolves #18603

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

readline, stream, doc, test

@vsemozhetbyt @mcollina

mcollina

Good work, LGTM

mcollina · 2018-02-21T17:00:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13312/

mscdex · 2018-02-21T17:50:05Z

test/parallel/test-readline-async-iterators.js

+const file_content = 'abc123\n' +
+                     '南越国是前203年至前111年存在于岭南地区的一个国家\n';
+
+fs.writeFile(filename, file_content);


Shouldn't this really be writeFileSync() to avoid any potential race conditions?

mscdex · 2018-02-21T17:50:29Z

test/parallel/test-readline-async-iterators.js

+tests().then(common.mustCall());
+
+process.on('exit', function() {
+  fs.unlinkSync(filename);


Can't this instead be moved to the common.mustCall() callback?

It can be removed entirely. Test files do not need to clean up the temp directory. Tests that use the temp directory clean it up themselves at the start of the test with tmpdir.refresh().

@Trott Is that worth to also clean up current fs tests in separate pr? example

targos · 2018-02-21T19:26:24Z

test/parallel/test-readline-async-iterators.js

+    let data = '';
+    for await (const k of rli) {
+      data += k;
+    }


I think it would be nice to check how many iterations of the loop there were

vsemozhetbyt · 2018-02-21T22:03:31Z

test/parallel/test-readline-async-iterators.js

+
+const common = require('../common');
+const fs = require('fs');
+const join = require('path').join;


A nit: destruction?

vsemozhetbyt · 2018-02-21T22:26:14Z

test/parallel/test-readline-async-iterators.js

+fs.writeFile(filename, file_content);
+
+async function tests() {
+  await (async function() {


A nit: arrow function?

vsemozhetbyt · 2018-02-21T22:34:39Z

Just a tiny doubt: the readline module works with strings only and users may get used to this (line in the listener argument is always a string). Should we demand from a user to use readable.setEncoding('utf8'); here? Maybe we can do this internally? BTW, the test does not use this call.

prog1dev · 2018-02-22T04:15:15Z

@vsemozhetbyt By demand you mean explicitly mention this in documentation?
I can set encoding in Interface.prototype[Symbol.asyncIterator] if you think this is a good thing to do.

devsnek · 2018-02-22T04:20:17Z

lib/readline.js

@@ -239,6 +241,14 @@ Interface.prototype.setPrompt = function(prompt) {
 };


+Interface.prototype[Symbol.asyncIterator] = function() {
+  emitExperimentalWarning('Interface[Symbol.asyncIterator]');


nit: in a project it may not be clear what Interface is

devsnek

lgtm with nits

vsemozhetbyt · 2018-02-22T10:02:04Z

@prog1dev Yes, documented advice.
Let us see what others think about it, I may be wrong.

vsemozhetbyt · 2018-02-23T15:59:44Z

@prog1dev I've compiled the branch and tested the slightly edited doc example code on Windows 7 x64.

readable.setEncoding('utf8'); seems do not affect the behavior (it is the same with it or without it).
It seems we have only one iteration with many lines glued together:

'use strict';

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

async function processLineByLine(readable) {
  // readable.setEncoding('utf8');
  const rli = readline.createInterface({
    input: readable,
    crlfDelay: Infinity
  });

  let i = 0;

  for await (const line of rli) {
    console.log(`Line ${++i}: ${line}`);
  }
}

processLineByLine(fs.createReadStream(__filename)).catch(console.error);

(node:3148) ExperimentalWarning: Interface[Symbol.asyncIterator] is an experimental feature. This feature could change at any time
Line 1: 'use strict';

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

async function processLineByLine(readable) {
  // readable.setEncoding('utf8');
  const rli = readline.createInterface({
    input: readable,
    crlfDelay: Infinity
  });

  let i = 0;

  for await (const line of rli) {
    console.log(`Line ${i}: ${line}`);
  }
}

processLineByLine(fs.createReadStream(__filename)).catch(console.error);

Dear reviewers, please chime in.

TimothyGu

As in @vsemozhetbyt’s example, the added method doesn’t work. readline emits 'line' events that needed to be listened for, not 'data' as is the case for readable streams. As such, the general stream async iterator cannot be used for readline.

The PR just gets an async iterator of the input stream. That completely skips the readline mechanism, and as such does not work.

prog1dev · 2018-02-23T17:07:45Z

@mcollina Looks like no matter how much lines file has there is only one iteration of for-await-of. Here is code example:

async function print(readable) {
  readable.setEncoding('utf8');
  let data = '';
  let iterations = 0;
  for await (const k of readable) {
    data += k;
    iterations++;
  }
  console.log(data);
  console.log(iterations);
}

print(fs.createReadStream('file')).catch(console.log);

Is this behaviour intentional? Its different for readable streams in object mode
I guess its because fs.createReadStream('file').read() returns null

vsemozhetbyt · 2018-02-23T23:25:21Z

It seems the longing readline to be more like streams has some backing, for example #16178.

TBH, when I started to learn Node.js, using readline for a file processing seemed to me a bit hacky, with all the terminal abundant API to be ignored. And it becomes more awkward with things like crlfDelay: Infinity.

But this is a pretty basic tool for text file processing to be outsourced to more handy userland modules. And this async API would be a big step towards making readline more usable outside the CLI domain till we will have something more concise.

mcollina

-1 based on @vsemozhetbyt exmaple.

mcollina · 2018-02-23T23:54:09Z

Is this behaviour intentional? Its different for readable streams in object mode

I'm pretty sure this is is because of internal buffering. You should try reading a different file. This is an experimental feature, and I would be surprise if it does not have bugs. Feel free to send PRs to fix those.

BridgeAR · 2018-03-02T01:45:36Z

Ping @prog1dev

prog1dev · 2018-03-03T17:18:58Z

@BridgeAR Im working on this in my spare time

vsemozhetbyt · 2018-05-05T17:26:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14676/

TimothyGu · 2018-05-05T23:52:18Z

lib/readline.js

+    return null;
+
+  return this._buffer.shift();
+};


Can we make this function private?

You mean to rename it to _read?

TimothyGu · 2018-05-05T23:53:13Z

lib/readline.js

+
+  this._enableBuffer = true;
+  this.close();
+  this.input.setEncoding('utf8');


It's definitely not super friendly when a module like readline changes stream settings, especially when we only do it with async iteration. We should add this feature without it.

TimothyGu · 2018-05-05T23:54:16Z

lib/readline.js

+  emitExperimentalWarning('readline Interface[Symbol.asyncIterator]');
+
+  this._enableBuffer = true;
+  this.close();


We definitely should not close the readline interface when we start iterating. This will emit events like 'close' that can be very much misleading to users.

In order for it to work stream must be in paused mode so I can get data with stream.read() calls. So instead of closing I guess I can pause it with this.input.pause()

TimothyGu · 2018-05-05T23:54:59Z

lib/readline.js

+Interface.prototype[Symbol.asyncIterator] = function() {
+  emitExperimentalWarning('readline Interface[Symbol.asyncIterator]');
+
+  this._enableBuffer = true;


I don't see this ever getting restored after the completion of iteration, and I feel like it should be. Correct me if I'm wrong :)

TimothyGu · 2018-05-05T23:59:05Z

There is still a lot of logic shared between ReadableAsyncIterator and ReadlineAsyncIterator, like the entire

      this[kLastResolve] = null;
      this[kLastReject] = null;
      this[kError] = null;
      this[kEnded] = false;
      this[kLastPromise] = null;

chunk in the constructor, and also the this[kHandlePromise] function. BaseAsyncIterator could definitely have some more common code.

mcollina · 2018-05-07T13:45:30Z

lib/readline.js

@@ -77,6 +80,8 @@ function Interface(input, output, completer, terminal) {
  this.isCompletionEnabled = true;
  this._sawKeyPress = false;
  this._previousKey = null;
+  this._enableBuffer = false;


Can you please add a comment on what this does? Also, I would prefer if this was a Symbol instead.

mcollina · 2018-05-07T13:48:14Z

Let me toy with an idea.. how about we implement this on top of a PassThrough instead?
Basically we listen to the 'line' event and we write to a PassThrough with objectMode: true. Then, we use that to create the AsyncIterator. It seems we are doubling some streams machinery here.

jasnell · 2018-10-17T17:20:01Z

What's the status on this?

prog1dev · 2018-10-17T17:31:16Z

@jasnell Stalled. Maybe later this week I can check this PassThrough idea. Also at that time I could not find an easy way to solve this #18904 (comment) issue

TimothyGu · 2018-10-22T21:30:04Z

@prog1dev Would it be okay with you if I were to take over this PR?

prog1dev · 2018-10-22T21:58:02Z

@TimothyGu Sure go on

TimothyGu · 2018-10-27T01:45:42Z

New PR opened at #23916.

Co-authored-by: Ivan Filenko <[email protected]> Fixes: nodejs#18603 Refs: nodejs#18904

Co-authored-by: Ivan Filenko <[email protected]> Fixes: nodejs#18603 Refs: nodejs#18904 PR-URL: nodejs#23916 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

Co-authored-by: Ivan Filenko <[email protected]> Fixes: #18603 Refs: #18904 PR-URL: #23916 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

Co-authored-by: Ivan Filenko <[email protected]> Fixes: nodejs#18603 Refs: nodejs#18904 PR-URL: nodejs#23916 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

Co-authored-by: Ivan Filenko <[email protected]> Fixes: #18603 Refs: #18904 PR-URL: #23916 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Feb 21, 2018

mcollina approved these changes Feb 21, 2018

View reviewed changes

mcollina added dont-land-on-v4.x labels Feb 21, 2018

mscdex reviewed Feb 21, 2018

View reviewed changes

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 21, 2018

targos approved these changes Feb 21, 2018

View reviewed changes

vsemozhetbyt reviewed Feb 21, 2018

View reviewed changes

vsemozhetbyt approved these changes Feb 21, 2018

View reviewed changes

devsnek reviewed Feb 22, 2018

View reviewed changes

devsnek approved these changes Feb 22, 2018

View reviewed changes

TimothyGu previously requested changes Feb 23, 2018

View reviewed changes

vsemozhetbyt added the experimental Issues and PRs related to experimental features. label Feb 23, 2018

vsemozhetbyt mentioned this pull request Feb 23, 2018

test: test-assert.js fails locally #18967

Closed

mcollina requested changes Feb 23, 2018

View reviewed changes

prog1dev force-pushed the readline/add_support_for_async_iteration branch from e3532d8 to 3468ed9 Compare March 12, 2018 20:20

stream: move common async iteration logic to base_async_iterator

2f5c558

prog1dev force-pushed the readline/add_support_for_async_iteration branch from 716e6f1 to 2f5c558 Compare May 5, 2018 17:21

TimothyGu reviewed May 5, 2018

View reviewed changes

mcollina reviewed May 7, 2018

View reviewed changes

Trott removed the dont-land-on-v4.x label Jul 19, 2018

jasnell added stalled Issues and PRs that are stalled. and removed dont-land-on-v9.x labels Aug 12, 2018

devsnek mentioned this pull request Sep 22, 2018

Readline: emit data events along with current line events #23032

Closed

TimothyGu added the wip Issues and PRs that are still a work in progress. label Oct 25, 2018

TimothyGu mentioned this pull request Oct 27, 2018

readline: add support for async iteration #23916

Closed

4 tasks

TimothyGu added a commit to TimothyGu/node that referenced this pull request Nov 20, 2018

readline: add support for async iteration

179dd7b

Co-authored-by: Ivan Filenko <[email protected]> Fixes: nodejs#18603 Refs: nodejs#18904

prog1dev closed this Nov 20, 2018

prog1dev deleted the readline/add_support_for_async_iteration branch November 20, 2018 23:46

readline: add support for async iteration #18904

readline: add support for async iteration #18904

Conversation

prog1dev commented Feb 21, 2018 • edited Loading

Checklist

Affected core subsystem(s)

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prog1dev Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

vsemozhetbyt commented Feb 21, 2018 • edited Loading

prog1dev commented Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

devsnek left a comment

Choose a reason for hiding this comment

vsemozhetbyt commented Feb 22, 2018

vsemozhetbyt commented Feb 23, 2018 • edited Loading

TimothyGu left a comment

Choose a reason for hiding this comment

prog1dev commented Feb 23, 2018 • edited Loading

vsemozhetbyt commented Feb 23, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Feb 23, 2018

BridgeAR commented Mar 2, 2018

prog1dev commented Mar 3, 2018 • edited Loading

vsemozhetbyt commented May 5, 2018

Choose a reason for hiding this comment

prog1dev May 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prog1dev May 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented May 5, 2018

Choose a reason for hiding this comment

mcollina commented May 7, 2018

jasnell commented Oct 17, 2018

prog1dev commented Oct 17, 2018

TimothyGu commented Oct 22, 2018

prog1dev commented Oct 22, 2018

TimothyGu commented Oct 27, 2018

prog1dev commented Feb 21, 2018 •

edited

Loading

prog1dev Feb 22, 2018 •

edited

Loading

vsemozhetbyt Feb 21, 2018 •

edited

Loading

vsemozhetbyt commented Feb 21, 2018 •

edited

Loading

prog1dev commented Feb 22, 2018 •

edited

Loading

vsemozhetbyt commented Feb 23, 2018 •

edited

Loading

prog1dev commented Feb 23, 2018 •

edited

Loading

prog1dev commented Mar 3, 2018 •

edited

Loading

prog1dev May 20, 2018 •

edited

Loading

prog1dev May 20, 2018 •

edited

Loading