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.

Sign up for GitHub

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

Jump to bottom

readline: use Date.now() and move test to parallel #18563

Closed
wants to merge 3 commits into from
Closed

readline: use Date.now() and move test to parallel #18563

wants to merge 3 commits into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Feb 4, 2018 •
edited
Loading

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 •
edited
Loading

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

lpinca
lpinca reviewed Feb 4, 2018
View reviewed changes
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.

@apapirovski
readline: use Date.now() and move test to parallel
eda075e 
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
fixup: don't early assign the value
81ce2d1
@apapirovski apapirovski force-pushed the patch-readline-timer-now branch from b9378f3 to 81ce2d1 Compare February 4, 2018 21:25
@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

Ok, this is no longer WIP as the stress test CI is clean: https://ci.nodejs.org/job/node-stress-single-test/1774/nodes=osx1010/console

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

@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
@apapirovski @BridgeAR
readline: use Date.now() and move test to parallel
af5632e 
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
@apapirovski @MylesBorins
readline: use Date.now() and move test to parallel
3f73b7f 
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
@apapirovski @MylesBorins
readline: use Date.now() and move test to parallel
2ee0850 
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
@apapirovski @MylesBorins
readline: use Date.now() and move test to parallel
36332eb 
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
Merged
gibfahn pushed a commit that referenced this pull request Apr 13, 2018
@apapirovski @gibfahn
readline: use Date.now() and move test to parallel
1faae90 
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
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@apapirovski @MayaLekova
readline: use Date.now() and move test to parallel
075d088 
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
Reviewers

@lpinca lpinca lpinca approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@apapirovski @maclover7 @BridgeAR @addaleax @lpinca @gibfahn @nodejs-github-bot