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

readline: remove max limit of crlfDelay #13497

Closed
wants to merge 8 commits into from
Closed

readline: remove max limit of crlfDelay #13497

wants to merge 8 commits into from

Conversation

Azard
Copy link
Contributor

@Azard Azard commented Jun 6, 2017

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.

@nodejs-github-bot nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Jun 6, 2017
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be clearer as:

will be coerced to 100 or greater

@Fishrock123
Copy link
Contributor

Fishrock123 commented Jun 6, 2017

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

@Fishrock123
Copy link
Contributor

@Azard
Copy link
Contributor Author

Azard commented Jun 6, 2017

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
Copy link
Contributor

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

@Azard
Copy link
Contributor Author

Azard commented Jun 6, 2017

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

lib/readline.js Outdated
@@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infinity >>> 0 is 0 😢

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you fixed this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I fixed it

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@aqrln
Copy link
Contributor

aqrln commented Jun 21, 2017

@@ -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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@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
Copy link
Contributor Author

Azard commented Jul 30, 2017

@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?

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@addaleax
Copy link
Member

@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
Copy link
Contributor

@refack
Copy link
Contributor

refack commented Jul 30, 2017

@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
  ...

lib/readline.js Outdated
@@ -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 ?
Copy link
Contributor

@refack refack Jul 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++.

Copy link
Contributor Author

@Azard Azard Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@refack Could you help me to start CI ?

@Azard
Copy link
Contributor Author

Azard commented Jul 30, 2017

@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
Copy link
Contributor

refack commented Jul 31, 2017

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@Azard
Copy link
Contributor Author

Azard commented Jul 31, 2017

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

@addaleax
Copy link
Member

Landed in b8e0a5e, thank you!

@addaleax addaleax closed this Jul 31, 2017
addaleax pushed a commit that referenced this pull request Jul 31, 2017
PR-URL: #13497
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 1, 2017
PR-URL: #13497
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax addaleax mentioned this pull request Aug 2, 2017
@MylesBorins
Copy link
Contributor

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
Copy link
Contributor Author

Azard commented Aug 17, 2017

@MylesBorins I have raised PR #14899

MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
Backport-PR-URL: #14899
PR-URL: #13497
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants