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

timers: handle unrefed intervals in clearTimeout() #9606

Closed
wants to merge 1 commit into from
Closed

timers: handle unrefed intervals in clearTimeout() #9606

wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Nov 14, 2016

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

timers

Description of change

This commit clears the _repeat property of all timer objects in clearTimeout(). This prevents intervals passed to clearTimeout() from being rearmed.

Fixes: #9561 (although it does not directly address wrap->object() issue described in #9561 (comment))

R= @bnoordhuis

@cjihrig
timers: handle unrefed intervals in clearTimeout()
1b8efe6 
This commit clears the _repeat property of all timer objects in
clearTimeout(). This prevents intervals passed to clearTimeout()
from being rearmed.
@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Nov 14, 2016
@Fishrock123
Copy link
Contributor

I don't think this is an exhaustive patch.

unenroll() should cancel any timer callbacks without fail and Timer#close() should always stop the _handle without fail.

@Fishrock123
Copy link
Contributor

See #9561 (comment)

@Fishrock123 Fishrock123 mentioned this pull request Nov 18, 2016
Merged
3 tasks
@cjihrig cjihrig closed this Nov 18, 2016
@cjihrig cjihrig deleted the 9561 branch November 18, 2016 21:13
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Nov 22, 2016
@Fishrock123
timers: use consistent checks for canceled timers
3f1e38c 
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>
Fishrock123 added a commit that referenced this pull request Nov 22, 2016
@Fishrock123
timers: use consistent checks for canceled timers
02c2bf7 
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: #9606
Fixes: #9561
PR-URL: #9685
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Dec 21, 2016
@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
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@Fishrock123 @MylesBorins
timers: use consistent checks for canceled timers
553d95d 
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: #9606
Fixes: #9561
PR-URL: #9685
Reviewed-By: Rich Trott <rtrott@gmail.com>

 Conflicts:
	lib/timers.js
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@Fishrock123 @MylesBorins
timers: use consistent checks for canceled timers
8bb66cd 
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: #9606
Fixes: #9561
PR-URL: #9685
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@Fishrock123 @MylesBorins
timers: use consistent checks for canceled timers
c349fb3 
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: #9606
Fixes: #9561
PR-URL: #9685
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@Fishrock123 @MylesBorins
timers: use consistent checks for canceled timers
d98d089 
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: #9606
Fixes: #9561
PR-URL: #9685
Reviewed-By: Rich Trott <rtrott@gmail.com>

 Conflicts:
	lib/timers.js
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
@cjihrig @Fishrock123 @nodejs-github-bot