timer: the this keyword is do always refer to global #4839

Closed
wants to merge 1 commit into
from

Projects

None yet

6 participants

@AndreasMadsen
Member

The this keyword in timer callbacks was very inconsistent, normally
this would refer to a internal timer object and if setImmediate was
called with arguments this would refer to the global object. This
commit normalizes what the this keyword refer to by always calling
the callback with null. E.q. callback.apply(null, args)

@bnoordhuis
Member

I think I'm okay with the PR but have you measured the performance impact? Your change means an extra anonymous function in the fast path.

@trevnorris

I wouldn't like to loose the ability to use this within setImmediate to point to the timer. It provides a very simple way to self cancel without needing to store an external reference. Example:

setInterval(function() {
  console.log('here');
  clearInterval(this);
}, 1000);

If consistency is the point then I'd suggest the only necessary change would be:

diff --git a/lib/timers.js b/lib/timers.js
index 68890ab..9890505 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -324,7 +324,7 @@ exports.setImmediate = function(callback) {
     args = Array.prototype.slice.call(arguments, 1);

     immediate._onImmediate = function() {
-      callback.apply(null, args);
+      callback.apply(immediate, args);
     };
   }
@AndreasMadsen
Member

@bnoordhuis no, didn't think of that, here you go:

$git checkout timeout-fix
$make && ./node benchmark/misc/timers.js 
misc/timers.js thousands=500 type=depth: 224.58
misc/timers.js thousands=500 type=breadth: 679.58

$git checkout master
$make && ./node benchmark/misc/timers.js
misc/timers.js thousands=500 type=depth: 236.68
misc/timers.js thousands=500 type=breadth: 812.77

I don't think much can be done about it, i tried using a makeCallback function just like in the fs module, but that did only give worse performance.

@trevnorris Well thats undocumented API, and if it where to be documented it would be inconsistent with the DOM API:

... the this keyword for the called function will be set to the window (or global) object, ...
source: https://developer.mozilla.org/en-US/docs/DOM/window.setTimeout#Explanation

The advantage would also be very small since the this keyword is lost when calling another asynchronous method and it would then need to be stored in a variable, which is no different from var key = setInterval().

@bnoordhuis
Member

Other committers, opinions? Correctness is important but if it slows down things considerably, well...

@isaacs
isaacs commented Feb 26, 2013

I somewhat like @trevnorris's approach better. Let's just make setImmediate with args consistent with setTimeout and setInterval.

@isaacs
isaacs commented Feb 26, 2013

Overall, I think the "this" value of setTimeout functions is not something anyone actually cares about, so if a change slows things down, then no.

@AndreasMadsen AndreasMadsen timers: consistent this keyword in setImmediate
When calling setImmediate with extra arguments the this keyword in the callback
would refer to the global object, but when not calling setImmediate with extra
arguments this would refer to the returned handle object.

This commit fixes that inconsistency so its always set handle object. The
handle object was chosen for performance reasons.
0732bb0
@AndreasMadsen
Member

@isaacs thats fine. But I doubt anyone would have cared since node v0.8 was slower:

node8 benchmark/misc/timers.js
misc/timers.js thousands=500 type=depth: 186.94
misc/timers.js thousands=500 type=breadth: 614.87

I have pushed a new commit there fixes the setImmediate issue and adds a test. For historical reasons here is the old commit: AndreasMadsen/node@5457bff

@dougwilson

For my opinion, the reason the standard specifies to use the global object as this is simply because there is no other good value. It's as good as if they left it unspecified (but of course specifying it makes them consistent). I doubt people will actually be using this to mean the global (I'd think people would only be using this in that way accidentally). 🌵

@dougwilson

By the way, HTML5 specifies that setTimeout returns an integer greater than zero, but node returns an object. It can conceivably be argued that since the specification is regarding a setTimeout property on window, it doesn't apply to node's setTimeout, as the object it is a property of does not follow the window interface specification. In other words, I don't think it matters what this is. 🍎

@trevnorris

@AndreasMadsen awesome. and thanks for the additional tests.

@isaacs
isaacs commented Mar 8, 2013

Ok, last call for 0.10. Can another committer weigh in on this? I have a profound lack of caring about this in setTimeout callbacks. @bnoordhuis @indutny @TooTallNate @sblom @piscisaureus

@TooTallNate

Consistency is good. +1

@AndreasMadsen
Member

Here is the the original "usecase", perhaps it helps in the decision process.

I had to debug this problem:

function Something() {
  this.timer = setTImeout(this.method); /* forgot `.bind()` here */ 
}

Something.prototype.method = function () {
  console.log(this);
};

new Something()

and it was quite confusing that console.log(this) looked like what the Something object should have from the setTimeout call (it had all the _idle* properties), and it took some annoying time to discover that it wasn't the Something object, but the this object from setTImeout there wasn't what I expected (global).

tl;td
I cared very much about the wrong this object in setTimeout when I had to debug this problem, but now that I know setTimeout is wired I really don't care. I would stil prefer that the original solution landed, but I didn't want to be involved in a big pointless discussion so made the setImmediate fix.

And if you guys want speed there is already a pull request for that (#4193).

@isaacs
isaacs commented Mar 9, 2013

Yeah, consistency is good. Ok. Landed on 7becf15.

@isaacs isaacs closed this Mar 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment