FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Port from joyent/node: `timers: fix timeout when added in timer's callback` by Fishrock123 · Pull Request #2232 · nodejs/node · GitHub

/ node Public

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

Closed
Fishrock123 wants to merge 3 commits into
nodejs:masterfrom
Fishrock123:port-timer-fix
ConversationCommits (3)ChecksFiles changed
Closed

Port from joyent/node: timers: fix timeout when added in timer's callback#2232
Fishrock123 wants to merge 3 commits into
nodejs:masterfrom
Fishrock123:port-timer-fix

Conversation

Fishrock123 commented Jul 24, 2015

Copy link
Copy Markdown
Contributor

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

cc @misterdjules / @bnoordhuis

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 <fedor@indutny.com>

Conflicts:
	lib/timers.js
	test/common.js
Fishrock123 added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jul 24, 2015
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 reviewed Jul 24, 2015
View reviewed changes
Comment thread test/common.js Outdated

thefourtheye Jul 24, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Fishrock123 Jul 24, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oops, that was a merge conflict mis-resolve.

whitlockjc commented Jul 24, 2015

Copy link
Copy Markdown
Contributor

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

Fishrock123 commented Jul 24, 2015

Copy link
Copy Markdown
Contributor Author

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

Fishrock123 commented Jul 27, 2015

Copy link
Copy Markdown
Contributor Author

Updated, PTAL @misterdjules / @bnoordhuis

bnoordhuis reviewed Jul 27, 2015
View reviewed changes
Comment thread test/common.js Outdated

bnoordhuis Jul 27, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Date.now() here and two lines up?

Fishrock123 Jul 27, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

bnoordhuis Jul 27, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Jul 27, 2015

Copy link
Copy Markdown
Member

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

Fishrock123 commented Jul 28, 2015

Copy link
Copy Markdown
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 reviewed Jul 28, 2015
View reviewed changes
Comment thread test/common.js Outdated

thefourtheye Jul 28, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Fishrock123 Jul 28, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah nice, true.

snorp mentioned this pull request Aug 7, 2015
Closed

jasnell commented Aug 23, 2015

Copy link
Copy Markdown
Member

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

Fishrock123 commented Aug 23, 2015

Copy link
Copy Markdown
Contributor Author

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

Fishrock123 mentioned this pull request Sep 26, 2015
Closed

misterdjules commented Oct 1, 2015

Copy link
Copy Markdown

@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 commented Jan 11, 2016

Copy link
Copy Markdown
Contributor Author

closing in favor of #3063

misterdjules mentioned this pull request Jun 29, 2016
Closed
3 tasks

whitlockjc commented Jul 7, 2016

Copy link
Copy Markdown
Contributor

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

Back | FazBrowse Home | New Git URL