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

Fix timers cancel in interval v4.x #10365

Closed
wants to merge 2 commits into from
Closed

Fix timers cancel in interval v4.x #10365

wants to merge 2 commits into from

Conversation

Fishrock123
Copy link
Contributor

@Fishrock123 Fishrock123 commented Dec 21, 2016 •
edited
Loading

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)

timers

Description of change

Backport of #9685 to v4.x

I added another check and testcase because there is another related bug prior to c8c2544

cc @thealphanerd

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

@Fishrock123
timers: use consistent checks for canceled timers
9c9277b 
Previously not all codepaths set `timer._idleTimeout = -1` for canceled
or closed timers, and not all codepaths checked it either.

Unenroll uses this to say that a timer is indeed closed and it is the
closest thing there is to an authoritative source for this.

Refs: nodejs#9606
Fixes: nodejs#9561
PR-URL: nodejs#9685
Reviewed-By: Rich Trott <rtrott@gmail.com>

 Conflicts:
	lib/timers.js
@Fishrock123 Fishrock123 added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. v4.x labels Dec 21, 2016
@nodejs-github-bot nodejs-github-bot added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. v4.x labels Dec 21, 2016
@Fishrock123 Fishrock123 mentioned this pull request Dec 21, 2016
Merged
3 tasks
@Fishrock123 Fishrock123 force-pushed the fix-timers-cancel-in-interval-v4.x branch from ec26d82 to f37b38c Compare December 21, 2016 00:13
@MylesBorins
Copy link
Contributor

landed in ae2eff2...c444119

@MylesBorins
Copy link
Contributor

LGTM

MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@Fishrock123 @MylesBorins
timers: bail from intervals if _repeat is bad
759e8fd 
PR-URL: #10365
Ref: #9685

Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@Fishrock123 @MylesBorins
timers: bail from intervals if _repeat is bad
0be437a 
PR-URL: #10365
Ref: #9685

Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 21, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Fishrock123 @MylesBorins @nodejs-github-bot