Fix timer .unref() handle management once and for all#1330
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
MemberAuthor
indutny commented Apr 2, 2015
lib/timers.js Outdated
Contributor
There was a problem hiding this comment.
I think this should probably be:
functiondestroyOnTimeout(){if(!this.owner._repeat&&this.owner._idleNext._idleNext===this.owner)this.owner.close();}MemberAuthor
There was a problem hiding this comment.
Perhaps, but may be it should not be there at all. I'll figure out the proper way of doing it in a bit.
Fishrock123 added the timers 
Contributor
brycebaril commented Apr 2, 2015
Built locally and tried out the test cases, did not appear fix the repro cases in #1151 or #1264 #1264 is a different timer issue, but the test case there is also a reliable test case for leaked timers -- if they leak it never exits. @thlorenz has also been doing a lot of research around these various timers issues and may want to take a peek here |
Contributor
Fishrock123 commented Apr 3, 2015
@brycebaril yes, #1264 is different.* This (in general) fixes timer's #1264 has to do with Thornsten's research into uv keeping the thread loop open. Edit: * probably.. timers is the real worm can though |
Fishrock123 changed the title .unref() handle management once and for allContributor
brycebaril commented Apr 3, 2015
@Fishrock123 what I'm trying to say is this patch doesn't fix the problem The two fixes pulled from joyent/node improve behavior, then commit indutny@c93bb3a makes things worse. #1264 is a different issue, but the test itself is good at illustrating the leaked handles in addition to to the uv issue. I added a few more This pull request without commit indutny@c93bb3a: The assertion error is because there is no fix for #1264, but we can't even get that far if the handles are leaked. with indutny@c93bb3a: You can see the As for the test for #1151: With this pull request change but without indutny@c93bb3a: With indutny@c93bb3a: Here's the diff with the extra diff --git a/lib/timers.js b/lib/timers.js index 225e2af..fbe6ce8 100644 --- a/lib/timers.js+++ b/lib/timers.js@@ -4,7 +4,15 @@ const Timer = process.binding('timer_wrap').Timer; const L = require('_linklist'); const assert = require('assert').ok; const util = require('util'); -const debug = util.debuglog('timer');+const _debug = util.debuglog('timer');+function debug(){+ var args = [];+ for (var i = 0; i < arguments.length; i++){+ args[i] = arguments[i];+ }+ args[0] = Date.now() + ": " + args[0];+ _debug.apply(null, args);+} const kOnTimeout = Timer.kOnTimeout | 0; const kOnUnrefTimeout = Timer.kOnUnrefTimeout | 0; @@ -272,14 +280,18 @@ exports.setInterval = function(callback, repeat){return timer; function wrapper(){+ debug('wrapper start'); timer._repeat.call(this); // If timer is unref'd (or was - it's permanently removed from the list.) if (this._handle){+ debug('have a _handle (already unrefd)'); this._handle.start(repeat, 0)} else{+ debug('no _handle, setting active'); timer._idleTimeout = repeat; exports.active(timer)} + debug('wrapper end'); } }; @@ -311,6 +323,7 @@ function destroyOnTimeout(){Timeout.prototype.unref = function(){if (this._handle){+ debug('forwarding unref to hidden _handle'); this._handle.unref()} else if (typeof(this._onTimeout) === 'function'){var now = Timer.now(); @@ -322,6 +335,7 @@ Timeout.prototype.unref = function(){// Prevent running cb again when unref() is called during the same cb if (this._called && !this._repeat) return; + debug('creating hidden _handle'); this._handle = new Timer(); this._handle.owner = this; this._handle[kOnUnrefTimeout] = destroyOnTimeout; |
Contributor
brycebaril commented Apr 3, 2015
@indutny the |
MemberAuthor
indutny commented Apr 3, 2015
@brycebaril I'm afraid that tests you are talking about are unrelated to what I'm trying to fix here. |
indutnyforce-pushed the fix/timers-once-and-for-all branch from c93bb3a to 13cd8e2CompareMemberAuthor
indutny commented Apr 3, 2015
@Fishrock123@trevnorris PTAL, I've fixed it in a proper way. |
Contributor
brycebaril commented Apr 3, 2015
@indutny they DO very much have to do with the timer leak, and your latest commits to the branch get us back into a workable state. 😄 The failed assert is a separate issue, so disregard that -- if the timers are leaked you can never get to the assert. Before your latest commits the |
MemberAuthor
indutny commented Apr 3, 2015
@brycebaril sorry, I'm terrible english speaker. Does it work now? |
Contributor
brycebaril commented Apr 3, 2015
@indutny yes, it works now |
MemberAuthor
indutny commented Apr 3, 2015
Yaaay! |
Contributor
brycebaril commented Apr 3, 2015
Here's a swag at a test that demonstrates the timers being leaked which is fixed by this PR: vartimers=require('timers');varassert=require('assert');varoriginal=timers.active;varcalls=0;timers.active=functionwrapActive(item){calls++;original.call(null,item);}vari=setInterval(functionnoop(){},10);i.unref();setTimeout(function(){// timers.active should be called exactly twice, once for the first setInterval,// and once for the setTimeoutassert.equal(calls,2);},50); |
MemberAuthor
indutny commented Apr 3, 2015
@Fishrock123 added test. cc @brycebaril |
Contributor
brycebaril commented Apr 3, 2015
Test looks good, confirmed it is fixed by this patch. |
MemberAuthor
indutny commented Apr 3, 2015
Yay, thank you! |
The destructor isn't being called for timers that have been unref'd. Fixes: nodejs/node-v0.x-archive#8364
This change fixes a regression introduced by commit 0d05123, which contained a typo that would cause every unrefd interval to fire only once. Fixes: nodejs/node-v0.x-archive#8900 Reviewed-By: Timothy J Fontaine <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-by: Trevor Norris <[email protected]>
Partially revert 776b73b. Following code crashes after backported timer leak fixes: ```javascript var timer = setInterval(function(){clearInterval(timer)}, 10); timer.unref(); ``` Note that this is actually tested in a `test-timers-unref.js`, and is crashing only with 776b73b. Calling `clearInterval` leads to the crashes in case of `.unref()`ed timers, and might lead to a extra timer spin in case of regular intervals that was closed during the interval callback. All of these happens because `.unref()`ed timer has it's own `_handle` and was used after the `.close()`.
indutnyforce-pushed the fix/timers-once-and-for-all branch from c5c33aa to 452571bCompareMemberAuthor
indutny commented Apr 3, 2015
Contributor
There was a problem hiding this comment.
This will run this._handle.close() which runs uv_close(wrap->handle__, OnClose). Are we sure that the list of timers attached to this TimerWrap instance is empty before this runs?
MemberAuthor
There was a problem hiding this comment.
No list attached here for sure.
Contributor
trevnorris commented Apr 3, 2015
One question, but other than that LGTM. |
trevnorris added a commit that referenced this pull request Apr 3, 2015
The destructor isn't being called for timers that have been unref'd. Fixes: nodejs/node-v0.x-archive#8364 PR-URL: #1330 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
indutny added a commit that referenced this pull request Apr 3, 2015
Partially revert 776b73b. Following code crashes after backported timer leak fixes: ```javascript var timer = setInterval(function(){clearInterval(timer)}, 10); timer.unref(); ``` Note that this is actually tested in a `test-timers-unref.js`, and is crashing only with 776b73b. Calling `clearInterval` leads to the crashes in case of `.unref()`ed timers, and might lead to a extra timer spin in case of regular intervals that was closed during the interval callback. All of these happens because `.unref()`ed timer has it's own `_handle` and was used after the `.close()`. PR-URL: #1330 Reviewed-by: Trevor Norris <[email protected]>
indutny added a commit that referenced this pull request Apr 3, 2015
PR-URL: #1330 Reviewed-by: Trevor Norris <[email protected]>
indutny added a commit that referenced this pull request Apr 3, 2015
PR-URL: #1330 Reviewed-by: Trevor Norris <[email protected]>
Contributor
Fishrock123 commented Apr 3, 2015
\o/ |
Fishrock123 added a commit that referenced this pull request Apr 6, 2015
Notable changes: * npm: upgrade npm to 2.7.5. See the npm CHANGELOG.md for details. Includes two important security fixes. https://github.com/npm/npm/blob/master/CHANGELOG.md#v275-2015-03-26 * openssl: preliminary work has been done for an upcoming upgrade to OpenSSL 1.0.2a #1325 (Shigeki Ohtsu). See #589 for additional details. * timers: a minor memory leak when timers are unreferenced was fixed, alongside some related timers issues #1330 (Fedor Indutny). This appears to have fixed the remaining leak reported in #1075. * android: it is now possible to compile io.js for Android and related devices #1307 (Giovanny Andres Gongora Granada).
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See: #1152
See: #1151
See: #1075 (may be)