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: use Date.now() and move test to parallel #18563

Closed

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Feb 4, 2018

The readline module wants a truthy time while using Timer.now() doesn't necessarily guarantee that early on in the process' life. It also doesn't actually resolve the timing issues experienced in #14681. Instead, this PR fixes the related tests and moves them back to parallel.

This needs a stress test & CI (hence the WIP), but I can't seem to run one from a different repository anymore as used to be possible...

CI:
https://ci.nodejs.org/job/node-test-pull-request/12927/ (green)
https://ci.nodejs.org/job/node-test-pull-request/12930/ (green)
https://ci.nodejs.org/job/node-test-pull-request/12931/ (green)
https://ci.nodejs.org/job/node-test-pull-request/12932/
https://ci.nodejs.org/job/node-test-pull-request/12933/
Stress test CI:
https://ci.nodejs.org/job/node-stress-single-test/1774/ (OS X)

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

readline

@nodejs-github-bot nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Feb 4, 2018
@maclover7
Copy link
Contributor

Hmm, it looks like readline used to use Date.now(), but was changed to timer_wrap on purpose via #14681, cc @Trott

@apapirovski
Copy link
Member Author

apapirovski commented Feb 4, 2018

@maclover7 I'm aware. I'm like 99.999% positive that change wasn't strictly correct. Timer.now() updates internal uv time so it shouldn't fix the bug on its own. Indeed, running those tests in parallel proves that the bug exists with both Timer.now() and Date.now(). Hence the actual fix to those tests in this PR.

(Also, strictly speaking Timer.now() could return 0 early in the process' life so it's not really ideal for using in readline which wants a truthy time.)

lib/readline.js Outdated
var string = this._decoder.write(b);
if (this._sawReturnAt &&
now() - this._sawReturnAt <= this.crlfDelay) {
now - this._sawReturnAt <= this.crlfDelay) {
Copy link
Member

Choose a reason for hiding this comment

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

On second thought, now is a costant, so isn't this always 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's constant within the scope of the function but the case it's accounting for is when _normalWrite is called twice in a row, once with \n and then with \r.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha.

Copy link
Member Author

Choose a reason for hiding this comment

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

That said, this did make me rethink caching the value like I did. It doesn't make any sense and it's an unnecessary call in many situations. I'll fix that up.

Copy link
Member

Choose a reason for hiding this comment

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

Also it's definitely easier to grok now.

The readline module wants a truthy time while using Timer.now() doesn't
necessarily guarantee that early on in the process' life. It also
doesn't actually resolve the timing issues experienced in an earlier
issue. Instead, this PR fixes the related tests and moves them back
to parallel.

Refs: nodejs#14674
@apapirovski apapirovski changed the title [WIP] readline: use Date.now() and move test to parallel readline: use Date.now() and move test to parallel Feb 4, 2018
@apapirovski
Copy link
Member Author

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 6, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 7, 2018
The readline module wants a truthy time while using Timer.now() doesn't
necessarily guarantee that early on in the process' life. It also
doesn't actually resolve the timing issues experienced in an earlier
issue. Instead, this PR fixes the related tests and moves them back
to parallel.

Refs: nodejs#14674

PR-URL: nodejs#18563
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Feb 7, 2018

Landed in af5632e 🎉

@BridgeAR BridgeAR closed this Feb 7, 2018
@apapirovski apapirovski deleted the patch-readline-timer-now branch February 7, 2018 16:49
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
The readline module wants a truthy time while using Timer.now() doesn't
necessarily guarantee that early on in the process' life. It also
doesn't actually resolve the timing issues experienced in an earlier
issue. Instead, this PR fixes the related tests and moves them back
to parallel.

Refs: #14674

PR-URL: #18563
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
The readline module wants a truthy time while using Timer.now() doesn't
necessarily guarantee that early on in the process' life. It also
doesn't actually resolve the timing issues experienced in an earlier
issue. Instead, this PR fixes the related tests and moves them back
to parallel.

Refs: #14674

PR-URL: #18563
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
The readline module wants a truthy time while using Timer.now() doesn't
necessarily guarantee that early on in the process' life. It also
doesn't actually resolve the timing issues experienced in an earlier
issue. Instead, this PR fixes the related tests and moves them back
to parallel.

Refs: #14674

PR-URL: #18563
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
gibfahn pushed a commit that referenced this pull request Apr 13, 2018
The readline module wants a truthy time while using Timer.now() doesn't
necessarily guarantee that early on in the process' life. It also
doesn't actually resolve the timing issues experienced in an earlier
issue. Instead, this PR fixes the related tests and moves them back
to parallel.

Refs: #14674

PR-URL: #18563
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
The readline module wants a truthy time while using Timer.now() doesn't
necessarily guarantee that early on in the process' life. It also
doesn't actually resolve the timing issues experienced in an earlier
issue. Instead, this PR fixes the related tests and moves them back
to parallel.

Refs: nodejs#14674

PR-URL: nodejs#18563
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants