timers: assure setTimeout callback only runs once by silverwind · Pull Request #1231 · nodejs/node · GitHub

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.

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.

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?

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

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

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

Yeah, tests did pass. Will research later.

Please add this._called = false to the constructor.

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.

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.

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

That test still fails with my current patch.

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

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

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

@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.

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

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

LGTM please land

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

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

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 timers, setImmediate(), setInterval(), and 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

timers: assure setTimeout callback only runs once by silverwind · Pull Request #1231 · nodejs/node · GitHub

Repository navigation

timers: assure setTimeout callback only runs once - #1231

timers: assure setTimeout callback only runs once#1231silverwind wants to merge 5 commits intonodejs:v1.xnodejs/node:v1.xfrom silverwind:unref-in-callback-fixsilverwind/node:unref-in-callback-fixCopy head branch name to clipboard

Conversation

silverwind commented Mar 21, 2015

Uh oh!

Fishrock123 Mar 21, 2015

Choose a reason for hiding this comment

Uh oh!

silverwind Mar 21, 2015

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Mar 21, 2015

Uh oh!

silverwind commented Mar 21, 2015

Uh oh!

Fishrock123 commented Mar 21, 2015

Uh oh!

Fishrock123 commented Mar 21, 2015

Uh oh!

silverwind commented Mar 21, 2015

Uh oh!

trevnorris Mar 23, 2015

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Mar 23, 2015

Uh oh!

silverwind commented Mar 23, 2015

Uh oh!

Fishrock123 commented Mar 25, 2015

Uh oh!

silverwind commented Mar 25, 2015

Uh oh!

Fishrock123 commented Mar 25, 2015

Uh oh!

silverwind commented Mar 25, 2015

Uh oh!

silverwind commented Mar 25, 2015

Uh oh!

silverwind commented Mar 25, 2015

Uh oh!

trevnorris commented Mar 25, 2015

Uh oh!

Fishrock123 commented Mar 26, 2015

Uh oh!

Fishrock123 commented Mar 26, 2015

Uh oh!

Fishrock123 commented Mar 26, 2015

Uh oh!

silverwind commented Mar 26, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timers: assure setTimeout callback only runs once#1231
silverwind wants to merge 5 commits into
nodejs:v1.xfrom
silverwind:unref-in-callback-fix