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

setTimeout can fire twice #1191

Closed
trevnorris opened this issue Mar 18, 2015 · 10 comments
Closed

setTimeout can fire twice #1191

trevnorris opened this issue Mar 18, 2015 · 10 comments
Labels
confirmed-bug Issues with confirmed bugs. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@trevnorris
Copy link
Contributor

Test to reproduce:

$ ./iojs -e "setTimeout(function() { console.log('hi'); this.unref(); this.ref(); }, 10)"
hi
hi

I haven't had the time to investigate, but want to make sure this issue can be tracked.

@trevnorris trevnorris added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Mar 18, 2015
@Fishrock123
Copy link
Contributor

For posterity this was found during irc discussion with @trevnorris last night regarding #1152 -- http://logs.libuv.org/io.js/2015-03-17#23:26:55.816

@mscdex mscdex added the confirmed-bug Issues with confirmed bugs. label Mar 18, 2015
@silverwind
Copy link
Contributor

Looks a unref() is enough to trigger it:

$ NODE_DEBUG=timer iojs -e "setTimeout(function() { console.log('hi'); this.unref(); }, 10)"
TIMER 46607: timeout callback 1
TIMER 46607: now: 17345754
hi
TIMER 46607: unenroll
TIMER 46607: unenroll: list empty
TIMER 46607: 1 list empty
hi

Why does Timeout.prototype.unref run the timer again (through this._handle.start(delay, 0);, I assume) ?

cc @tjfontaine @bnoordhuis

@silverwind
Copy link
Contributor

Or rather, I think the issue is with the unenroll list being empty.

@Fishrock123
Copy link
Contributor

It seems to me as though it doesn't expect you to unref during it's own callback. I.e. it does not check that the callback has already been or is being called..

@silverwind
Copy link
Contributor

I'm pretty certain that unenroll isn't working as expected in this case. The timer code is pretty convoluted I have to say.

@Fishrock123
Copy link
Contributor

I'm not sure unref needs to make a handle in the case that the callback is already being called.

@silverwind
Copy link
Contributor

Yeah, it's too late to unref a already fired timer, that unref() should be noop. The problem now is how do we know the Timeout._onTimeout fired? add something liked Timeout._fired = true and check that Timeout.prototype.unref ? Any other suggestions?

@silverwind
Copy link
Contributor

Hold on, I got an idea.

@silverwind
Copy link
Contributor

Nope, I think I can't do without a tracking property on the timeout. 😢

silverwind added a commit that referenced this issue Mar 26, 2015
Calling this.unref() during the callback of SetTimeout caused the
callback to get executed twice because unref() didn't expect to be
called during that time and did not stop the ref()ed Timeout but
did start a new timer. This commit prevents the new timer creation
when the callback was already called.

Fixes: #1191
Reviewed-by: Trevor Norris <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
PR_URL: #1231
@silverwind
Copy link
Contributor

Fixed in b0f8e30

silverwind added a commit that referenced this issue Mar 26, 2015
Calling this.unref() during the callback of SetTimeout caused the
callback to get executed twice because unref() didn't expect to be
called during that time and did not stop the ref()ed Timeout but
did start a new timer. This commit prevents the new timer creation
when the callback was already called.

Fixes: #1191
Reviewed-by: Trevor Norris <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
PR-URL: #1231
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

No branches or pull requests

4 participants