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.

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

timers: assure setTimeout callback only runs once #1231

Closed
wants to merge 5 commits into from

Conversation

silverwind
Copy link
Contributor

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

@silverwind silverwind changed the title timer: assure setTimeout callback only run once timer: assure setTimeout callback only runs once Mar 21, 2015
@silverwind silverwind added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Mar 21, 2015
@silverwind silverwind changed the title timer: assure setTimeout callback only runs once timers: assure setTimeout callback only runs once Mar 21, 2015
This fixes an edge case where running this.unref() during the callback caused the
callback to get executed multiple times.
@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Fishrock123
Copy link
Contributor

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
Copy link
Contributor Author

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

@Fishrock123
Copy link
Contributor

@Fishrock123
Copy link
Contributor

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

@silverwind
Copy link
Contributor Author

Yeah, tests did pass. Will research later.

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

Choose a reason for hiding this comment

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

Please add this._called = false to the constructor.

@trevnorris
Copy link
Contributor

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
Copy link
Contributor Author

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.

@rvagg rvagg mentioned this pull request Mar 25, 2015
@Fishrock123
Copy link
Contributor

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
Copy link
Contributor Author

That test still fails with my current patch.

@Fishrock123
Copy link
Contributor

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

@silverwind
Copy link
Contributor Author

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

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

@silverwind
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor

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

@Fishrock123
Copy link
Contributor

LGTM please land

@Fishrock123
Copy link
Contributor

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

@Fishrock123
Copy link
Contributor

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

silverwind added a commit that referenced this pull request Mar 26, 2015
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
Copy link
Contributor Author

Thanks, landed in b0f8e30

@silverwind silverwind closed this Mar 26, 2015
silverwind added a commit that referenced this pull request Mar 26, 2015
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
rvagg added a commit that referenced this pull request Mar 31, 2015
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants