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: key interval delay for \r & \n #8109

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@princejwesley
Contributor

princejwesley commented Aug 15, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

readline

Description of change

Emit two line events when there is a delay between
CR('\r') and LF('\n')

@cjihrig

View changes

Show outdated Hide outdated test/parallel/test-readline-interface.js
@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Aug 15, 2016

Member

@princejwesley Could you give an example as to how to induce the problem this fixes?

\r\n (\n after \r) is a grapheme iirc aka one character

Member

Fishrock123 commented Aug 15, 2016

@princejwesley Could you give an example as to how to induce the problem this fixes?

\r\n (\n after \r) is a grapheme iirc aka one character

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley

princejwesley Aug 15, 2016

Contributor

@Fishrock123

This is how I reproduced in macOS. Run the below snippet and hit enter/return for (\r) and ctrl + j for (\n). You can try with repl too.

const readline = require('readline');

const rli = new readline.Interface({
    input: process.stdin,
    output: process.stdout,
    terminal: true
});
let count = 0;
rli.on('line', function(line) {
    count++;
    console.log(`line event triggered ${count}`);
});

Edit: \r\n - two characters. Here is the interesting history behind it

Contributor

princejwesley commented Aug 15, 2016

@Fishrock123

This is how I reproduced in macOS. Run the below snippet and hit enter/return for (\r) and ctrl + j for (\n). You can try with repl too.

const readline = require('readline');

const rli = new readline.Interface({
    input: process.stdin,
    output: process.stdout,
    terminal: true
});
let count = 0;
rli.on('line', function(line) {
    count++;
    console.log(`line event triggered ${count}`);
});

Edit: \r\n - two characters. Here is the interesting history behind it

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 15, 2016

Member

It may be better to make the delay period configurable

Member

jasnell commented Aug 15, 2016

It may be better to make the delay period configurable

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Aug 15, 2016

Contributor

I think if we're going to add a delay, it should definitely be configurable (with some sensible default but allowing to opt out altogether -- perhaps with a 0 value).

Contributor

mscdex commented Aug 15, 2016

I think if we're going to add a delay, it should definitely be configurable (with some sensible default but allowing to opt out altogether -- perhaps with a 0 value).

@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley

princejwesley Aug 15, 2016

Contributor

@mscdex Delay is introduced to differentiate whether its a system that emits \r\n for a single newline or a user or both. Setting delay as zero will nullify this fix.

In nutshell, this PR tries to figure out whether \n is an individual event or the tail of \r(emitted by the system for newline).

Current experience,

~ 🙈 node
> // I hit enter - that emits CR(\r)
> // now I hit `ctrl + j ` to emit LF(\n) - no newline
Contributor

princejwesley commented Aug 15, 2016

@mscdex Delay is introduced to differentiate whether its a system that emits \r\n for a single newline or a user or both. Setting delay as zero will nullify this fix.

In nutshell, this PR tries to figure out whether \n is an individual event or the tail of \r(emitted by the system for newline).

Current experience,

~ 🙈 node
> // I hit enter - that emits CR(\r)
> // now I hit `ctrl + j ` to emit LF(\n) - no newline
@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Aug 15, 2016

Contributor

@princejwesley I understand, but I think it would be useful to be able to explicitly tell readline what to check for instead of trying to guess by adding a delay, whether that's setting 0 for the delay or setting some other option/flag.

Contributor

mscdex commented Aug 15, 2016

@princejwesley I understand, but I think it would be useful to be able to explicitly tell readline what to check for instead of trying to guess by adding a delay, whether that's setting 0 for the delay or setting some other option/flag.

@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley

princejwesley Aug 15, 2016

Contributor

@mscdex I'll update the PR with configurable option. Shall I use crLfDelay or any better name?

Contributor

princejwesley commented Aug 15, 2016

@mscdex I'll update the PR with configurable option. Shall I use crLfDelay or any better name?

@mscdex

View changes

Show outdated Hide outdated lib/readline.js
@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Aug 15, 2016

Contributor

@princejwesley I don't really have a preference on the name. Although personally I think if it starts with 'CRLF', that prefix would look better lowercased (crlf) instead of mixed cased (crLf).

Contributor

mscdex commented Aug 15, 2016

@princejwesley I don't really have a preference on the name. Although personally I think if it starts with 'CRLF', that prefix would look better lowercased (crlf) instead of mixed cased (crLf).

@princejwesley

This comment has been minimized.

Show comment
Hide comment
Contributor

princejwesley commented Aug 16, 2016

@Trott Trott added the semver-minor label Aug 16, 2016

@jasnell

View changes

Show outdated Hide outdated doc/api/readline.md
@jasnell

View changes

Show outdated Hide outdated lib/readline.js
@jasnell

View changes

Show outdated Hide outdated lib/readline.js
@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley
Contributor

princejwesley commented Aug 20, 2016

@yorkie

View changes

Show outdated Hide outdated test/parallel/test-readline-interface.js

princejwesley added some commits Aug 15, 2016

readline: key interval delay for \r & \n
Emit two line events when there is a delay between
CR('\r') and LF('\n')
@yorkie

This comment has been minimized.

Show comment
Hide comment
@yorkie

yorkie Aug 24, 2016

Member

LGTM :)

Member

yorkie commented Aug 24, 2016

LGTM :)

@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley
Contributor

princejwesley commented Aug 26, 2016

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 26, 2016

Member

LGTM

Member

jasnell commented Aug 26, 2016

LGTM

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Aug 26, 2016

Contributor

LGTM

Contributor

mscdex commented Aug 26, 2016

LGTM

@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley
Contributor

princejwesley commented Aug 28, 2016

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 30, 2016

Member

build bot failure on the last CI run... trying one final time for green: https://ci.nodejs.org/job/node-test-pull-request/3897/

Member

jasnell commented Aug 30, 2016

build bot failure on the last CI run... trying one final time for green: https://ci.nodejs.org/job/node-test-pull-request/3897/

@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley

princejwesley Aug 31, 2016

Contributor

@jasnell failed again 😞

Contributor

princejwesley commented Aug 31, 2016

@jasnell failed again 😞

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 31, 2016

Member

@nodejs/build @jbergstroem ... can someone take a look to see what's happening with the arm build bot on this?

Member

jasnell commented Aug 31, 2016

@nodejs/build @jbergstroem ... can someone take a look to see what's happening with the arm build bot on this?

@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley
Contributor

princejwesley commented Sep 2, 2016

@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley

princejwesley Sep 5, 2016

Contributor

@jasnell I see full of red & unstable CI jobs. I'll trigger build after few days to get green build

Contributor

princejwesley commented Sep 5, 2016

@jasnell I see full of red & unstable CI jobs. I'll trigger build after few days to get green build

@jbergstroem

This comment has been minimized.

Show comment
Hide comment
@jbergstroem

jbergstroem Sep 5, 2016

Member

We've fixed the arm issues; disk-related. Please try again!

Member

jbergstroem commented Sep 5, 2016

We've fixed the arm issues; disk-related. Please try again!

@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley

princejwesley Sep 8, 2016

Contributor

@jbergstroem last two build attempts - unstable. can you take a look?

Contributor

princejwesley commented Sep 8, 2016

@jbergstroem last two build attempts - unstable. can you take a look?

@jbergstroem

This comment has been minimized.

Show comment
Hide comment
@jbergstroem

jbergstroem Sep 8, 2016

Member

@princejwesley 3959,3960 looked good though! Flaky is fine -- it means a test is known to fail.

Member

jbergstroem commented Sep 8, 2016

@princejwesley 3959,3960 looked good though! Flaky is fine -- it means a test is known to fail.

princejwesley added a commit that referenced this pull request Sep 8, 2016

readline: key interval delay for \r & \n
Emit two line events when there is a delay between
CR('\r') and LF('\n').

Introduced a new option `crlfDelay`. 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.

PR-URL: #8109
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@princejwesley

This comment has been minimized.

Show comment
Hide comment
@princejwesley

princejwesley Sep 8, 2016

Contributor

Landed in a634554

Contributor

princejwesley commented Sep 8, 2016

Landed in a634554

Fishrock123 added a commit that referenced this pull request Sep 14, 2016

readline: key interval delay for \r & \n
Emit two line events when there is a delay between
CR('\r') and LF('\n').

Introduced a new option `crlfDelay`. 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.

PR-URL: #8109
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>

@Azard Azard referenced this pull request Jun 6, 2017

Closed

readline: remove max limit of crlfDelay #13497

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment