timers: assure setTimeout callback only runs once #1231

silverwind · 2015-03-21T17:00:13Z

This fixes #1191 by adding a property to track if the callback was run, and if so, prevents it from running again during a .unref().

I woul've loved to do it without the property, but couldn't find any way to find out whether the callback already ran.

r=@bnoordhuis @trevnorris

This fixes an edge case where running this.unref() during the callback caused the callback to get executed multiple times.

Fishrock123 · 2015-03-21T17:06:56Z

lib/timers.js

@@ -305,6 +306,9 @@ Timeout.prototype.unref = function() {
  if (this._handle) {
    this._handle.unref();
  } else if (typeof(this._onTimeout) === 'function') {
+    // Prevent running callback multiple times
+    // when unref() is called during the callback
+    if (this._called) return;


I think it's probably best to just do this before if (this._handle) {, but I'm not 100% sure it matters.

Yes, I was unsure here too, but since I only wanted to prevent the _onTimeout call, this seemed right.

Fishrock123 · 2015-03-21T17:15:25Z

Was also trying fixes for this.

I assumed this should also affect intervals, but it doesn't seem to..
Maybe https://github.com/iojs/io.js/blob/v1.x/lib/timers.js#L277-L278 prevents that from happening?

silverwind · 2015-03-21T17:26:30Z

Yeah, didn't see it on intervals either. Right now, I fail to even find where .start() is defined.

Fishrock123 · 2015-03-21T17:56:39Z

@silverwind https://github.com/iojs/io.js/blob/v1.x/src/timer_wrap.cc#L73-L80

Fishrock123 · 2015-03-21T17:59:05Z

Also, I think it needs a timer._called = false; here for intervals. Did the tests pass on the current patch?

silverwind · 2015-03-21T18:00:45Z

Yeah, tests did pass. Will research later.

trevnorris · 2015-03-23T16:31:00Z

lib/timers.js

@@ -85,6 +85,7 @@ function listOnTimeout() {
        if (domain)
          domain.enter();
        threw = true;
+        first._called = true;


Please add this._called = false to the constructor.

trevnorris · 2015-03-23T16:33:57Z

Overall I think this is a decent fix for the apparent issue at hand. Timers should be revisited in more detail later to cleanup the implementation. Minus one comment, LGTM.

silverwind · 2015-03-23T21:15:08Z

Thanks for the review, I'll update with your suggestion once I'm sure this doesn't affect setInterval. I might also revise some of the timer tests in process.

I agree timers should be cleaned up or be rewritten at some point. It's very hard to follow the code flow, and there's probably a few perf gains to be had too.

Fishrock123 · 2015-03-25T17:35:46Z

This is might be more related to #1152 / #1151 than initially thought.

Failing test:

var assert = require('assert');

setImmediate(function () {
  var count = 0;
  while (count++ < 1e7) {
    Math.random()
  }
});

var i = setTimeout(function () {
  this.unref();
  setImmediate(process.exit);
}, 10);

process.on('exit', function() {
  assert.strictEqual(process._getActiveHandles().length, 0);
});

Edit: updated test asset

silverwind · 2015-03-25T18:06:19Z

That test still fails with my current patch.

Fishrock123 · 2015-03-25T18:07:38Z

@silverwind hmm, don't let it hold this patch up though. It may or may not be related.

silverwind · 2015-03-25T18:08:49Z

Ah, wait, I derped the build appararently, retesting.

edit: nope, linked test still failing with a clean build, so probably unrelated.

silverwind · 2015-03-25T18:56:52Z

@Fishrock123 turns out unref() during the setInterval callback did fail with my patch (it kept the process running), the last test is for that case.

silverwind · 2015-03-25T19:49:20Z

Alright, I'm pretty confident that this should solve the setInterval issue (I identify a interval by the ._repeat prop), and it shouldn't cause new leaks by letting the unenroll call go through.

The added setInterval test doesn't assert anything and is only there to possibly timeout that test.

PTAL @Fishrock123 @trevnorris

trevnorris · 2015-03-25T21:38:29Z

Doesn't fix everything, but does it does fix known bugs. LGTM.

Fishrock123 · 2015-03-26T01:03:18Z

LGTM please land

Fishrock123 · 2015-03-26T01:04:17Z

Here's a CI for good measure though: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/374/

Fishrock123 · 2015-03-26T01:26:17Z

CI is happier than it looks. Those errors have been resolved since this pr's base.

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 <trev.norris@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> PR_URL: #1231

silverwind · 2015-03-26T06:34:06Z

Thanks, landed in b0f8e30

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 <trev.norris@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> PR-URL: #1231

Notable changes: * fs: corruption can be caused by fs.writeFileSync() and append-mode fs.writeFile() and fs.writeFileSync() under certain circumstances, reported in #1058, fixed in #1063 (Olov Lassus). * iojs: an "internal modules" API has been introduced to allow core code to share JavaScript modules internally only without having to expose them as a public API, this feature is for core-only #848 (Vladimir Kurchatkin). * timers: two minor problems with timers have been fixed: - Timer#close() is now properly idempotent #1288 (Petka Antonov). - setTimeout() will only run the callback once now after an unref() during the callback #1231 (Roman Reiss). * Windows: a "delay-load hook" has been added for compiled add-ons on Windows that should alleviate some of the problems that Windows users may be experiencing with add-ons in io.js #1251 (Bert Belder). * V8: minor bug-fix upgrade for V8 to 4.1.0.27. * npm: upgrade npm to 2.7.4. See npm CHANGELOG.md for details.

silverwind changed the title ~~timer: assure setTimeout callback only run once~~ timer: assure setTimeout callback only runs once Mar 21, 2015

silverwind added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Mar 21, 2015

silverwind changed the title ~~timer: assure setTimeout callback only runs once~~ timers: assure setTimeout callback only runs once Mar 21, 2015

timers: assure setTimeout callback only runs once

951168f

This fixes an edge case where running this.unref() during the callback caused the callback to get executed multiple times.

silverwind force-pushed the unref-in-callback-fix branch from c8489f7 to 951168f Compare March 21, 2015 17:02

Fishrock123 reviewed Mar 21, 2015
View reviewed changes

trevnorris reviewed Mar 23, 2015
View reviewed changes

rvagg mentioned this pull request Mar 25, 2015

Release proposal: v1.6.3 #1252

Closed

Fishrock123 mentioned this pull request Mar 25, 2015

timers: fix unref() memory leak #1152

Closed

silverwind added 2 commits March 25, 2015 19:19

Add _called to prototype

bc94f6c

Add test for unref during setInterval

a58d149

Let timer unenroll and prevent setInterval issues

c99318c

Add note to setInterval test

1a824b1

brycebaril mentioned this pull request Mar 26, 2015

unref'd timers running in beforeExit time #1264

Closed

silverwind closed this Mar 26, 2015

Fishrock123 mentioned this pull request Apr 9, 2015

test: windows ci timeouts related to child_process #1383

Closed

Fishrock123 mentioned this pull request May 29, 2015

Repeat unref'd interval persists program until next interval nodejs/node-v0.x-archive#7327

Closed

MarcelRaschke mentioned this pull request Nov 5, 2022

[Snyk] Security upgrade webpack-cli from 3.3.12 to 4.0.0 MarcelRaschke/node#18

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: assure setTimeout callback only runs once #1231

timers: assure setTimeout callback only runs once #1231

silverwind commented Mar 21, 2015

Fishrock123 Mar 21, 2015

silverwind Mar 21, 2015

Fishrock123 commented Mar 21, 2015

silverwind commented Mar 21, 2015

Fishrock123 commented Mar 21, 2015

Fishrock123 commented Mar 21, 2015

silverwind commented Mar 21, 2015

trevnorris Mar 23, 2015

trevnorris commented Mar 23, 2015

silverwind commented Mar 23, 2015

Fishrock123 commented Mar 25, 2015

silverwind commented Mar 25, 2015

Fishrock123 commented Mar 25, 2015

silverwind commented Mar 25, 2015

silverwind commented Mar 25, 2015

silverwind commented Mar 25, 2015

trevnorris commented Mar 25, 2015

Fishrock123 commented Mar 26, 2015

Fishrock123 commented Mar 26, 2015

Fishrock123 commented Mar 26, 2015

silverwind commented Mar 26, 2015

timers: assure setTimeout callback only runs once #1231

timers: assure setTimeout callback only runs once #1231

Conversation

silverwind commented Mar 21, 2015

Fishrock123 Mar 21, 2015

Choose a reason for hiding this comment

silverwind Mar 21, 2015

Choose a reason for hiding this comment

Fishrock123 commented Mar 21, 2015

silverwind commented Mar 21, 2015

Fishrock123 commented Mar 21, 2015

Fishrock123 commented Mar 21, 2015

silverwind commented Mar 21, 2015

trevnorris Mar 23, 2015

Choose a reason for hiding this comment

trevnorris commented Mar 23, 2015

silverwind commented Mar 23, 2015

Fishrock123 commented Mar 25, 2015

silverwind commented Mar 25, 2015

Fishrock123 commented Mar 25, 2015

silverwind commented Mar 25, 2015

silverwind commented Mar 25, 2015

silverwind commented Mar 25, 2015

trevnorris commented Mar 25, 2015

Fishrock123 commented Mar 26, 2015

Fishrock123 commented Mar 26, 2015

Fishrock123 commented Mar 26, 2015

silverwind commented Mar 26, 2015