Repository navigation
Should nextTick and timers accept context or parameters? #953
Description
Activity
Thanks! Looks like it's still open in node from 2013.
Thanks! Looks like it's still open in node from 2013.
That doesn't mean anything. I saw open reasonable feature requests dated 2010 :)
My opinion: we can make internal
nextTickwith this feature and try it out. Eventually it can be exposedIt would probably make the nextTick() code significantly more complex, though - and it's already pretty complex to start with. It's also extremely performance sensitive, it would be very easy to introduce performance regressions.
Looks pretty straightforward:
diff --git a/src/node.js b/src/node.js index f0bee17..e8a07b5 100644 --- a/src/node.js +++ b/src/node.js @@ -307,7 +307,8 @@ nextTickQueue.push({ callback: runMicrotasksCallback, - domain: null + domain: null, + context: null }); tickInfo[kLength]++; @@ -334,7 +335,7 @@ callback = tock.callback; threw = true; try { - callback(); + callback.call(tock.context); threw = false; } finally { if (threw) @@ -360,7 +361,7 @@ domain.enter(); threw = true; try { - callback(); + callback.call(tock.context); threw = false; } finally { if (threw) @@ -375,14 +376,15 @@ tickDone(); } - function nextTick(callback) { + function nextTick(callback, context) { // on the way out, don't bother. it won't get fired anyway. if (process._exiting) return; var obj = { callback: callback, - domain: process.domain || null + domain: process.domain || null, + context: context }; nextTickQueue.push(obj);
Yes, and doing the changes @vkurchatkin proposed will actually allow cleaning a bunch of
self = thiscode (in addition to being faster - although this issue isn't a perf request)I'm pretty sure I even put in a PR for this once because the symmetry with
setTimeout()was too temptingHowever,
nextTick()isn't even encouraged these days, it should be considered more of an internal feature and users should be generally opting forsetImmediate()which has all of this (doesn't it?).nextTick()needs to be fast and keeping it simple is going to help with that.So -1 from me.
@rvagg I'm not convinced
nextTickshould support a context or argument parameter but for what it's worth the issue was raised because I sawvar self = thisinside the core code base in several places and thought that it could be a nice way to clean up the code base.@benjamingr performance has always won over "clean code" in core so in general I think you might have a hard time cleaning anything up that has performance tradeoffs.
@rvagg it's actually much faster to pass context than to use a closure - this is actually an optimisation that several PRs (like the recent fs performance PR) utilize.
That said I have nothing solid at this stage to indicate that performance will improve because of this here and since that code is sensitive unless someone wants to do all the work involved in profiling it I'm afraid it indeed might be better to 'leave it be' at this stage.
It seems that timers already support this. So I'm closing this right now - something to consider in the future.
Currently, both timers and
nextTickonly take a callback argument (and some timers take a duration argument).It is beneficial to pass additional arguments and/or the context to timers. The io.js code base is littered with code like:
It's possible to either pass a context (
this) to these functions or to pass arguments like the DOM version does making the above:Or:
This can make cleaner code and save closure allocations - it also isn't slow like
.bind.Was this proposed before?
I realize the Timers API is locked (
nextTickis not afaict) but I'm wondering how people feel about this.