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

Port from joyent/node: timers: fix timeout when added in timer's callback #2232

Closed
wants to merge 3 commits into from
Closed

Port from joyent/node: timers: fix timeout when added in timer's callback #2232

wants to merge 3 commits into from

Conversation

Fishrock123
Copy link
Contributor

We'll probably also want to port nodejs/node-v0.x-archive#25763

cc @misterdjules / @bnoordhuis

@Fishrock123
timers: fix timeout when added in timer's callback
1facc67 
When a timer is added in another timer's callback, its underlying timer
handle will be started with a timeout that is actually incorrect.

The reason is that  the value that represents the current time is not
updated between the time the original callback is called and the time
the added timer is processed by timers.listOnTimeout. That leads the
logic in timers.listOnTimeout to do an incorrect computation that makes
the added timer fire with a timeout of scheduledTimeout +
timeSpentInCallback.

This change fixes that and make timers scheduled within other timers'
callbacks fire as expected.

Fixes: nodejs/node-v0.x-archive#9333
Fixes: nodejs/node-v0.x-archive#15447

PR: nodejs/node-v0.x-archive#17203
PR-URL: nodejs/node-v0.x-archive#17203
Reviewed-By: Fedor Indutny <[email protected]>

Conflicts:
	lib/timers.js
	test/common.js
@Fishrock123 Fishrock123 added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jul 24, 2015
@Fishrock123 Fishrock123 changed the title timers: fix timeout when added in timer's callback Port from joyent/node: timers: fix timeout when added in timer's callback Jul 24, 2015
thefourtheye
thefourtheye reviewed Jul 24, 2015
View reviewed changes
test/common.js
@@ -451,5 +451,9 @@ exports.fileExists = function(pathname) {
return true;
} catch (err) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch block and the function is not finished, I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, that was a merge conflict mis-resolve.

@whitlockjc
Copy link
Contributor

I wouldn't mind helping port nodejs/node-v0.x-archive#25763 if it's not already been done.

@Fishrock123
Copy link
Contributor Author

@whitlockjc I'll be yeah, I figured I'd probably let Julien review it over there first.

@Fishrock123
Copy link
Contributor Author

Updated, PTAL @misterdjules / @bnoordhuis

bnoordhuis
bnoordhuis reviewed Jul 27, 2015
View reviewed changes
test/common.js
exports.busyLoop = function busyLoop(time) {
var startTime = new Date().getTime();
var stopTime = startTime + time;
while (new Date().getTime() < stopTime);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Date.now() here and two lines up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, in the new patch joyent/node will be using Timer.now() -- what's the difference between it and Date.now()?

https://github.com/joyent/node/pull/25763/files#diff-8736c5cbff21e1dee18b0c86d3d2689dR226

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timer.now() has a fighting chance of fitting in an SMI (a tagged integer), the return value of Date.now() is always a heap-allocated double.

@bnoordhuis
Copy link
Member

LGTM with suggestions. The 100 ms timeout may end up being flaky on some of the CI machines.

@Fishrock123
Copy link
Contributor Author

Hmm, getting this with the patch locally:

=== release test-timers-unref-call ===                                         
Path: parallel/test-timers-unref-call
Command: out/Release/iojs /Users/Jeremiah/Documents/io.js/test/parallel/test-timers-unref-call.js
--- TIMEOUT ---

I'm guessing that test exposes something wrong with this patch, since the test is not present in joyent/node. Perhaps the same bug nodejs/node-v0.x-archive#25763 is attempting to rectify?

Test origin commit: ebf9f29

thefourtheye
thefourtheye reviewed Jul 28, 2015
View reviewed changes
test/common.js
@@ -453,3 +453,9 @@ exports.fileExists = function(pathname) {
return false;
}
};

exports.busyLoop = function busyLoop(time) {
var startTime = Date.now();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const? Also do we really need this? I mean we can directly use Date.now() in the following statement no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nice, true.

@snorp snorp mentioned this pull request Aug 7, 2015
Closed
@jasnell
Copy link
Member

jasnell commented Aug 23, 2015

@Fishrock123 ... what's the status on this one?

@Fishrock123
Copy link
Contributor Author

@jasnell was waiting on nodejs/node-v0.x-archive#25763

@Fishrock123 Fishrock123 mentioned this pull request Sep 26, 2015
Closed
@misterdjules
Copy link

@Fishrock123 The failure in test/parallel/test-timers-unref-call.js is due to a problem with the original change being ported that executes nested timers callbacks in the same tick.

Since test/parallel/test-timers-unref-call.js overrides Timer.now to always advance the current time, and because the timer delay is initially 1ms (but really one "tick" or one Timer.now call with this override), the new timers added by setInterval's callback are always ready to fire, and always do indefinitely.

nodejs/node-v0.x-archive#25763 fixes this problem.

@Fishrock123
Copy link
Contributor Author

closing in favor of #3063

@misterdjules misterdjules mentioned this pull request Jun 29, 2016
Closed
3 tasks
@whitlockjc
Copy link
Contributor

#3063 has been updated to include this change and the test/parallel/test-timers-blocking-callback.js test per @misterdjules' request.

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.
6 participants
@Fishrock123 @whitlockjc @bnoordhuis @jasnell @misterdjules @thefourtheye