timer: make sure timer is cleared by zhangzifa · Pull Request #4303 · nodejs/node · GitHub

When close is called in timer(set by setInterval) callback, active and
listOnTimeout will be called again which should not be called anymore.

When unenroll is called in timer(set by setInterval) callback, the timer
still works.

With this fix, the timer behaves the same as clearTimeout/clearInterval
is called in callback.

The timer (set by setInterval) should be disarmed when close/unenroll is called in callback.
The current implementation:

-- when close called in callback, an extra active is called.

> var i = setInterval(function(){console.log('1111'); i.close();}, 1111);
undefined
> TIMER 7917: timeout callback 15
TIMER 7917: now: 12423
TIMER 7917: 15 list empty
TIMER 7917: timeout callback 1111
TIMER 7917: now: 13514
1111
TIMER 7917: unenroll
TIMER 7917: unenroll: list empty
TIMER 7917: 1111 list empty
TIMER 7917: timeout callback 1111     //this callback execution is unnecessary.
TIMER 7917: now: 14627
TIMER 7917: 1111 list empty

-- when unenroll called, the timer is still working

> var T = require('timers');
...
> var i = setInterval(function(){console.log('1111'); T.unenroll(i);}, 1111);
> TIMER 7868: timeout callback 15
TIMER 7868: now: 30631
TIMER 7868: 15 list empty
TIMER 7868: timeout callback 1111
TIMER 7868: now: 31728
1111
TIMER 7868: unenroll
TIMER 7868: unenroll: list empty
TIMER 7868: 1111 list empty
TIMER 7868: timeout callback 1111
TIMER 7868: now: 32840
1111
TIMER 7868: unenroll
TIMER 7868: 1111 list empty
TIMER 7868: timeout callback 1111
TIMER 7868: now: 33953
1111
TIMER 7868: unenroll
TIMER 7868: 1111 list empty

(^C again to quit)
> TIMER 7868: timeout callback 1111
TIMER 7868: now: 35065
1111
TIMER 7868: unenroll
TIMER 7868: 1111 list empty

Can you add tests?

Ok, I will add test. And before that, i should fix another test. :-).
I did not notice the change between LTS and master for unref.

tests added, @mscdex

THANKS

I think it'd be better to forgo the use of child processes and just execute the timer tests directly.

Also, instead of using counters, you might use common.mustCall(fn, numOfExpectedCalls) to achieve the same effect with less code (e.g. setInterval(common.mustCall(function() { .... }), 11) to expect 1 call). Then inside your setTimeout() callback where you were checking the counters, just put a process.exit(0);.

But mustCall can't check how many times an internal function executed.

This fix is to make sure listOnTimeout(the same as exports.active) should not be triggered again after close/unenroll in setInterval callback.

Nit: there's an extra set or parenthesis here.

Also I think the check should prioritize timer._idleTimeout === -1, and only check ._repeat for legacy purposes.

I'm not entirely sure checking the handle is necessary. (Aside: Timer#unref() should probably check ._idleTimeout === -1, I think.)

Maybe the legacy logic to prioritize to check timer._repeat is result of calling clearTimeout/clearInterval more often than close/unenroll.

Check handle is necessary for unref() timer. (test case: test/parallel/test-timers-unrefd-interval-still-fires.js)

Maybe the legacy logic to prioritize to check timer._repeat is result of calling clearTimeout/clearInterval more often than close/unenroll.

@zhangzifa Right, neither close() no unenroll() are documented, and as such it is not suggested to use them.

Checking timer._idleTimeout === -1 however, is more in line with how the rest of the code works.

@Fishrock123
active(), close() and unenroll() should be removed, in my opinion. However, many modules call them.
https://github.com/search?utf8=%E2%9C%93&q=timers.unenroll&type=Code&ref=searchresults

I agree with you that check timer._idleTimeout === -1 is enough.

Someone optimized master branch and removed reuse(), it seems that this optimization should be removed and restore 'reuse()'.

@zhangzifa That search has a lot of false positives, GitHub search is not very good for that.

Someone optimized master branch and removed reuse(), it seems that this optimization should be removed and restore 'reuse()'.

Pardon? https://github.com/nodejs/node/blob/master/lib/timers.js#L122-L139

@Fishrock123 Sorry, my local copy is out of date.

My copy is before this PR.#3407

There's two extra sets or parenthesis here.

Also, could you make the change to prioritize timer._idleTimeout === -1? Thanks!

Checking timer._idleTimeout === -1 is enough, if _idleTimeout should be set to -1 in Timeout.prototype.close when this._handle is true.

timer._idleTimeout === -1 should always be true when a timer is closed/canceled/unenrolled/whatever to maintain consistency.

Yes, the old implementation missed to assign timer._idleTimeout as -1 when close an unrefd timer.

One of the filenames has a typo in it: callbak -> callback

Any reason we can't just do this?:

Move this file to /test/parallel
Add const common = require('../common'); to the top.
Surround the arrow function here with common.mustCall()
Get rid of test-timers-disarmed-in-callbak-not-fire-anymore.js

Never mind!

listOnTimeout will be called again which should not be called anymore. When unenroll is called in timer(set by setInterval) callback, the timer still works. With this fix, the timer behaves the same as clearTimeout/clearInterval is called in callback.

Hey @zhangzifa, I believe this would be your first commit to core if it gets in, welcome to the project! As I think you're finding out here, messing around with timers code can be tricky and involve a lot of edge-cases that are not necessarily covered by tests. Expect this PR to continue to be picked over by other collaborators before anything's ready to be merged. It's also possible that this might go another way, such as just deprecating close() and unenroll() (I think I'd prefer this option). Please persist with us while we try and get this one right!

@rvagg Happy new year! Yes, it's my first PR to this project. I'm puzzled why close() and unenroll() were exported, which i don't think necessary. Anyhow, before close() and unenroll() are removed, I think it's better to fix this fault as it is a logic fault. @Fishrock123 is refactoring timer (#4007), while this fault is still there. I am happy if another PR can fix this with a better implementation. Or @rvagg could you help involve someone who knows timer module well to review this?

Timeout#close() was add here: cd6122e (presumably reviewed by Bert)
timers.unenroll() predates probably 90+% of the current codebase. It was added before the browser-like *Timeout APIs. Here is where it ("timers"!) was first added to net.js ~6 years ago: ca862d7#diff-e6ef024c3775d787c38487a6309e491dR37

I wonder somewhat how useful the {un}enroll() / active() API possibly is. Is anyone actually using it for the means of efficiency like we do in core?

I figure we should probably patch this anyways, deprecating the latter, at least, would be quite an extended process I think.

Note that, before ~March 2015, we did not have /internal/ modules.

Hi @Fishrock123. Incredibly someone is using {un}enroll() / active() API.
This is the code from which I noticed the existence of them: https://github.com/felixge/node-mysql/blob/master/lib/protocol/Protocol.js . I agree with you how to handle this.

Interesting. Perhaps we should still look at Timeout#close()'s existence.

It seems not easy to find in which module Timeout#close() is used. I tried but failed.

What's the status on this one?

CI against current master: https://ci.nodejs.org/job/node-test-pull-request/4260/

@Fishrock123 ... any thoughts on this one?

The tests appear to pass on master. I am quite sure I fixed this in 3f1e38c.

The tests here should also not be necessary, I think.

@zhangzifa Thanks anyways for reporting this! Sorry it took so long. 😅

mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Dec 16, 2015

zhangzifa force-pushed the master branch from 2922892 to 29a3ac0 Compare December 16, 2015 07:03

zhangzifa changed the title disarm timer when close/unenroll called within callback disarmed timer by close/unenroll in callback should not fire anymore Dec 16, 2015

zhangzifa force-pushed the master branch from 031514c to 6269cba Compare December 16, 2015 07:28

Fishrock123 reviewed Dec 16, 2015
View reviewed changes

zhangzifa force-pushed the master branch from 6269cba to d1ebe75 Compare December 21, 2015 03:24

Fishrock123 reviewed Dec 21, 2015
View reviewed changes

zhangzifa force-pushed the master branch from 6035285 to 10dfe14 Compare December 22, 2015 08:56

zhangzifa changed the title disarmed timer by close/unenroll in callback should not fire anymore timer: disarmed timer in callback should not fire anymore Dec 22, 2015

zhangzifa force-pushed the master branch from 10dfe14 to fbefaa9 Compare December 22, 2015 09:18

zhangzifa changed the title timer: disarmed timer in callback should not fire anymore timer: make sure timer is cleared Dec 22, 2015

zhangzifa force-pushed the master branch from e0a620f to d5c407d Compare December 22, 2015 12:05

Trott reviewed Dec 23, 2015
View reviewed changes

zhangzifa force-pushed the master branch from be9f075 to a3d58db Compare December 24, 2015 03:42

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:01

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:22

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01

joyeecheung mentioned this pull request Dec 21, 2016

timers: use consistent checks for canceled timers #9685

Merged

3 tasks

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67 Compare February 1, 2017 01:00

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

Fishrock123 closed this Mar 24, 2017

timer: make sure timer is cleared by zhangzifa · Pull Request #4303 · nodejs/node · GitHub

Uh oh!

Uh oh!

timer: make sure timer is cleared - #4303

timer: make sure timer is cleared#4303zhangzifa wants to merge 1 commit intonodejs:masternodejs/node:masterfrom zhangzifa:masterzhangzifa/node:masterCopy head branch name to clipboard

Conversation

zhangzifa commented Dec 16, 2015

Uh oh!

mscdex commented Dec 16, 2015

Uh oh!

zhangzifa commented Dec 16, 2015

Uh oh!

zhangzifa commented Dec 16, 2015

Uh oh!

mscdex commented Dec 16, 2015

Uh oh!

zhangzifa commented Dec 16, 2015

Uh oh!

Fishrock123 Dec 16, 2015

Choose a reason for hiding this comment

Uh oh!

zhangzifa commented Dec 17, 2015

Uh oh!

Fishrock123 commented Dec 17, 2015

Uh oh!

zhangzifa commented Dec 18, 2015

Uh oh!

Fishrock123 commented Dec 18, 2015

Uh oh!

zhangzifa commented Dec 21, 2015

Uh oh!

Fishrock123 Dec 21, 2015

Choose a reason for hiding this comment

Uh oh!

zhangzifa Dec 22, 2015

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Dec 22, 2015

Uh oh!

zhangzifa commented Dec 22, 2015

Uh oh!

Trott commented Dec 23, 2015

Uh oh!

Trott Dec 23, 2015

Choose a reason for hiding this comment

Uh oh!

rvagg commented Jan 4, 2016

Uh oh!

zhangzifa commented Jan 4, 2016

Uh oh!

Fishrock123 commented Jan 4, 2016

Uh oh!

Fishrock123 commented Jan 4, 2016

Uh oh!

zhangzifa commented Jan 5, 2016

Uh oh!

Fishrock123 commented Jan 5, 2016

Uh oh!

zhangzifa commented Jan 8, 2016

Uh oh!

jasnell commented Mar 23, 2016

Uh oh!

imyller commented Sep 24, 2016

Uh oh!

jasnell commented Feb 28, 2017

Uh oh!

Fishrock123 commented Mar 24, 2017

Uh oh!

Fishrock123 commented Mar 24, 2017

Uh oh!

Fishrock123 commented Mar 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

timer: make sure timer is cleared#4303
zhangzifa wants to merge 1 commit into
nodejs:masterfrom
zhangzifa:master