readline: remove max limit of crlfDelay #13497

Azard · 2017-06-06T14:34:13Z

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

remove kMaxcrlfDelay
modify related docs
add one related unit test.

crlfDelay option of readline.createInterface is created by #8109 .
This pull request fix Interface.prototype._ttyWrite \r\n problem, but crlfDelay make some program use Interface.prototype._normalWrite behavior unexpect.

For example:

const rl = readline.createInterface({
  input: fs.createReadStream('big.csv', {
    encoding: 'utf16le',
  }),
  crlfDelay: 2000, // const kMaxcrlfDelay = 2000
});
rl.on('line', (line) => {
  // fuzzy string matching, take a lot of time
})

\r\n in csv file happen to be written in two different chunk, call them chunk A and chunk B.
chunk A end with \r , chunk B start with \n .
chunk A split into 1000 lines, each line event cost 3ms, after 3000ms, chunk B first line event emit with empty string '', because two chunk time gap over 2000ms, it consider \n is a new line.
The time gap over kMaxcrlfDelay is not rare.

kMaxcrlfDelay is meaningless in both tty write and normal write, remove it is a better design.

Trott · 2017-06-06T16:00:45Z

doc/api/readline.md

@@ -369,7 +369,7 @@ changes:
  * `crlfDelay` {number} If the delay between `\r` and `\n` exceeds
    `crlfDelay` milliseconds, both `\r` and `\n` will be treated as separate
    end-of-line input. Default to `100` milliseconds.
-    `crlfDelay` will be coerced to `[100, 2000]` range.
+    `crlfDelay` will be coerced not less than `100` milliseconds.


I think it would be clearer as:

will be coerced to 100 or greater

Fishrock123 · 2017-06-06T16:50:08Z

Does this allow it to be set to Infinity? If so, can we make sure to document & test that?

Fishrock123 · 2017-06-06T16:51:32Z

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

Azard · 2017-06-06T16:59:59Z

crlfDelay = Infinity is no problem, need I change https://github.com/Azard/node/blob/f26aded453aa643345e92cb34298b28155b4793f/test/parallel/test-readline-interface.js#L279 to Infinity , or add another unit test ?

Fishrock123 · 2017-06-06T17:03:28Z

@Azard A second test case would probably be preferable, I think.

Azard · 2017-06-06T17:48:32Z

I have fixed Infinity case, add unit test and update doc, local test pass (MacOS)

Azard · 2017-06-06T17:49:52Z

lib/readline.js

@@ -119,7 +119,8 @@ function Interface(input, output, completer, terminal) {
  this.input = input;
  this.historySize = historySize;
  this.removeHistoryDuplicates = !!removeHistoryDuplicates;
-  this.crlfDelay = Math.max(kMincrlfDelay, crlfDelay >>> 0);


Infinity >>> 0 is 0 😢

Looks like you fixed this?

yes, I fixed it

Fishrock123 · 2017-06-06T20:19:35Z

doc/api/readline.md

@@ -369,7 +369,7 @@ changes:
  * `crlfDelay` {number} If the delay between `\r` and `\n` exceeds
    `crlfDelay` milliseconds, both `\r` and `\n` will be treated as separate
    end-of-line input. Default to `100` milliseconds.
-    `crlfDelay` will be coerced to `[100, 2000]` range.
+    `crlfDelay` will be coerced to `100` or greater, `Infinity` is allowed.


Hmm, this could be clearer.

Maybe something along the lines of:

`crlfDelay` will be coerced to an Integer no less than `100`, unless it is `Infinity`.

aqrln · 2017-06-21T04:01:04Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/883/

addaleax · 2017-07-29T16:12:37Z

doc/api/readline.md

@@ -369,7 +369,8 @@ changes:
  * `crlfDelay` {number} If the delay between `\r` and `\n` exceeds
    `crlfDelay` milliseconds, both `\r` and `\n` will be treated as separate
    end-of-line input. Default to `100` milliseconds.
-    `crlfDelay` will be coerced to `[100, 2000]` range.
+    `crlfDelay` will be coerced to an Integer no less than `100`, unless it is
+    `Infinity`.


I would still call it coerced to a number instead of to an integer. There is no practical difference, and it allows you to drop the unless it is Infinity bit (that can still be mentioned, but the way this is written right now it’s a bit confusing). For example:

`crlfDelay` will be coerced to a number no less than `100`. It can be set to `Infinity`, in which case `\r` followed by `\n` will always be considered a single newline.

addaleax · 2017-07-29T16:14:03Z

@Azard This doesn’t need to wait for Node 9.0.0, if you feel like your pull request isn’t moving forward you can just bump this thread.

Azard · 2017-07-30T10:54:25Z

@addaleax I have updated docs.
"Bump this thread" means this PR can be merged with v8.3.x ? and how do I moving forward this PR?

addaleax · 2017-07-30T10:57:52Z

doc/api/readline.md

@@ -369,7 +369,7 @@ changes:
  * `crlfDelay` {number} If the delay between `\r` and `\n` exceeds
    `crlfDelay` milliseconds, both `\r` and `\n` will be treated as separate
    end-of-line input. Default to `100` milliseconds.
-    `crlfDelay` will be coerced to `[100, 2000]` range.
+    `crlfDelay` will be coerced to a number no less than `100`. It can be set to `Infinity`, in which case `\r` followed by `\n` will always be considered a single newline.


Sorry to comment again, but it would be great if you could wrap this line around at 80 characters … if not, that can also be done while landing the PR

@addaleax Thank you for detail review, I forgot it :) , now I wrapped.

addaleax · 2017-07-30T11:00:52Z

@Azard Thank you! “Bump this thread” means that you leave a quick comment here in this thread, asking what needs to be done (which at this point is nothing :)) for this to land or something like that.

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

Fishrock123 · 2017-07-30T16:00:38Z

CI yet again https://ci.nodejs.org/job/node-test-pull-request/9409/

refack · 2017-07-30T16:30:33Z

@Azard lint error caused by a new lint rule #14431. This could be fixed by lander, but always better to make lint (or make jslint) before pushing:

not ok 12 - /usr/home/iojs/build/workspace/node-test-linter/lib/readline.js
  ---
  message: Expected indentation of 4 spaces but found 19.
  severity: error
  data:
    line: 128
    column: 1
    ruleId: indent
  ...

refack · 2017-07-30T16:34:51Z

lib/readline.js

@@ -120,8 +119,8 @@ function Interface(input, output, completer, terminal) {
  this.input = input;
  this.historySize = historySize;
  this.removeHistoryDuplicates = !!removeHistoryDuplicates;
-  this.crlfDelay = Math.max(kMincrlfDelay,
-                            Math.min(kMaxcrlfDelay, crlfDelay >>> 0));
+  this.crlfDelay = crlfDelay === Infinity ?


Actually, could just collapse to Math.max(kMincrlfDelay, crlfDelay);

@refack This is because crlfDelay is number type (not integer), crlfDelay >>> 0 convert float to integer, but Infinity >>> 0 is 0.

being an integer might have been more important at some point in the past, but if you look at the current uses:
https://github.com/Azard/node/blob/d90647d0952091bd7053404945f1e01c57f5e98c/lib/readline.js#L412
and
https://github.com/Azard/node/blob/d90647d0952091bd7053404945f1e01c57f5e98c/lib/readline.js#L925
AFAICT treating it as just a number should work fine... (just need to make sure it's not passed to C++ and cast to int).

I fixed this with
this.crlfDelay = crlfDelay ? Math.max(kMincrlfDelay, crlfDelay) : kMincrlfDelay;
to make sure crlfDelay is not undefined, and add one unit test to check this.crlfDelay can be float number.
crlfDelay is a pure JavaScript variable, won't pass to C++.

@refack Could you help me to start CI ?

Azard · 2017-07-30T17:55:10Z

@refack Thank you for your guide, I forgot sync my branch before test.
Now I followed the lint rule and pass all local test.

refack · 2017-07-31T11:47:02Z

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

refack

💯

Azard · 2017-07-31T16:10:47Z

@refack It seems Ubuntu arm server down, and when this PR will be merged ? this is my first commit except doc fix.

addaleax · 2017-07-31T21:06:26Z

Landed in b8e0a5e, thank you!

PR-URL: #13497 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

MylesBorins · 2017-08-16T08:25:12Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Azard · 2017-08-17T17:24:32Z

@MylesBorins I have raised PR #14899

Backport-PR-URL: #14899 PR-URL: #13497 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Jun 6, 2017

Trott reviewed Jun 6, 2017

View reviewed changes

Azard commented Jun 6, 2017

View reviewed changes

princejwesley approved these changes Jun 6, 2017

View reviewed changes

Fishrock123 reviewed Jun 6, 2017

View reviewed changes

jasnell approved these changes Jun 13, 2017

View reviewed changes

Azard mentioned this pull request Jul 29, 2017

Tracking Issue: Node.js 9.0.0 #14391

Closed

addaleax approved these changes Jul 29, 2017

View reviewed changes

addaleax reviewed Jul 30, 2017

View reviewed changes

refack approved these changes Jul 30, 2017

View reviewed changes

refack reviewed Jul 30, 2017

View reviewed changes

Azard and others added 7 commits July 31, 2017 01:28

readline: remove max limit of crlfDelay

e1038fa

doc: api/readline.md clearer

056999e

readline: set crlfDelay to Infinity is allowed

b97c9c4

doc: clearer

f786414

update doc

43b78ec

wrap line around 80 chars

d7c1873

fix ident lint error

d90647d

crlfDelay is number, not convert to int

3d9c7a9

refack approved these changes Jul 31, 2017

View reviewed changes

addaleax closed this Jul 31, 2017

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

refack mentioned this pull request Aug 7, 2017

test: investigate flaky - parallel/test-readline-interface #14674

Closed

Azard mentioned this pull request Aug 7, 2017

test: fix test-readline-interface short delay #14677

Closed

3 tasks

MylesBorins added the backport-requested-v6.x label Aug 16, 2017

Azard mentioned this pull request Aug 17, 2017

[v6.x backport] readline: remove max limit of crlfDelay #14899

Closed

4 tasks

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Sep 19, 2017

MylesBorins mentioned this pull request Sep 20, 2017

v6.11.4 proposal #15506

Merged

vsemozhetbyt mentioned this pull request Oct 8, 2017

readline: crlfDelay and fs streams #16077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

readline: remove max limit of crlfDelay #13497

readline: remove max limit of crlfDelay #13497

Azard commented Jun 6, 2017 •

edited

Loading

Trott Jun 6, 2017

Fishrock123 commented Jun 6, 2017 •

edited

Loading

Fishrock123 commented Jun 6, 2017

Azard commented Jun 6, 2017 •

edited

Loading

Fishrock123 commented Jun 6, 2017

Azard commented Jun 6, 2017 •

edited

Loading

Azard Jun 6, 2017

Fishrock123 Jun 6, 2017

Azard Jun 7, 2017

Fishrock123 Jun 6, 2017

Azard Jun 7, 2017

aqrln commented Jun 21, 2017

addaleax Jul 29, 2017

addaleax commented Jul 29, 2017

Azard commented Jul 30, 2017

addaleax Jul 30, 2017

Azard Jul 30, 2017

addaleax commented Jul 30, 2017

Fishrock123 commented Jul 30, 2017

refack commented Jul 30, 2017

refack Jul 30, 2017 •

edited

Loading

Azard Jul 30, 2017

refack Jul 30, 2017

Azard Jul 31, 2017

Azard Jul 31, 2017 •

edited

Loading

Azard commented Jul 30, 2017 •

edited

Loading

refack commented Jul 31, 2017

refack left a comment

Azard commented Jul 31, 2017

addaleax commented Jul 31, 2017

MylesBorins commented Aug 16, 2017

Azard commented Aug 17, 2017

readline: remove max limit of crlfDelay #13497

readline: remove max limit of crlfDelay #13497

Conversation

Azard commented Jun 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Fishrock123 commented Jun 6, 2017 • edited Loading

Fishrock123 commented Jun 6, 2017

Azard commented Jun 6, 2017 • edited Loading

Fishrock123 commented Jun 6, 2017

Azard commented Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln commented Jun 21, 2017

Choose a reason for hiding this comment

addaleax commented Jul 29, 2017

Azard commented Jul 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jul 30, 2017

Fishrock123 commented Jul 30, 2017

refack commented Jul 30, 2017

refack Jul 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Azard Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Azard commented Jul 30, 2017 • edited Loading

refack commented Jul 31, 2017

refack left a comment

Choose a reason for hiding this comment

Azard commented Jul 31, 2017

addaleax commented Jul 31, 2017

MylesBorins commented Aug 16, 2017

Azard commented Aug 17, 2017

Azard commented Jun 6, 2017 •

edited

Loading

Fishrock123 commented Jun 6, 2017 •

edited

Loading

Azard commented Jun 6, 2017 •

edited

Loading

Azard commented Jun 6, 2017 •

edited

Loading

refack Jul 30, 2017 •

edited

Loading

Azard Jul 31, 2017 •

edited

Loading

Azard commented Jul 30, 2017 •

edited

Loading