Skip to content
This repository

Timers explode when date has been stubbed #3710

Closed
wants to merge 2 commits into from

9 participants

Austin Ben Noordhuis Magnus Wolffelt James Alexander Rosen Nathan Rajlich Isaac Z. Schlueter Trevor Norris Nodejs Jenkins Timothy J Fontaine
Austin

Timers in node all throw an exception when date has been stubbed, mocked, or modified and does not respond like a Date.

To allow for testing and to make times more self-relient the timers module now holds it's own copy of Date rather that referring to the global all over.

Use cases

  • Libraries like Timecop.js can be turned into a full npm modules
  • tests can have timeouts that do not freeze when time is modified
Isaac Z. Schlueter
Collaborator

Shouldn't this be global.Date? (In node modules this === exports.)

Austin

That should fix it

Ben Noordhuis

I don't know about this PR. The problem you're describing sounds like something that should be fixed in those test frameworks, not node core.

Austin

Here are a couple reasons I feel like this is a change for the better.

  1. This is how browsers work: in no browser environment I have encountered will you override Date and break core functions like set timeout. Providing the same interface will allow for better portability of code.

  2. The framework should be strong: I feel that a framework as fundamental as Node.js should not allow core functionality come to a screeching halt when something unrelated is overridden.

  3. Testing DateTime relient code is a pain: Without this small change Node would require you to create your own wrapper class to correctly test and stub date relient functions and maintain core functionality.

  4. The change has no downsides: It improve the stability of setTimeout and setInterval by removing the dependency on Date at run time this is a huge win, You are locking in what the module requires at compile time a user to modify the code to their hearts content.

  5. The change is small and testable. The fact that the change is testable will ensure that node does not lose the reliability of timers because of Date.

  6. Continues to help the testing and module community grow: I recently ported Timecop.js to be an npm module as well as have no Ruby dependency in the development enviroment. Both of these things are not possible without Timers being reliable not runtime dependent on Date.

  7. This pattern is happening elsewhere: https://github.com/visionmedia/mocha/blob/master/mocha.js#L1141
    Node should hold the same standard of reliability.

  8. Timers are not Dates: the fact that a Timer relies on Date is non consequential. A Timer should only care about the passage of time. Yes, the passage of time is calculated by the Date in this case, but if it could be, or was calculated another way, would that be a bad thing. No, because now Timers truly reflect their intention without exposing implementation details.

  9. To your point that the tests themselves should handle this. There is no clean way to handle stubbing of Date without creating a new interface for the Date object and then stubbing that. But that requires a new level of complexity for an application just to maintain a solid test suite. I feel strongly that this should be the responsibility of the framework, to provide the user with a stable core that is hard to untangle.

Magnus Wolffelt

I personally feel that "time travel" and so on, as described by Timecop, are concepts that are taking you in the wrong direction with regards to testing and testability. Code that wants to be testable, and relies on dates, timespans and such, should get all its time-related variables as parameters to constructors and functions.

Normally, creating a non-trivial object in a constructor or function - especially when it depends on global state like Date.. is a very bad idea IMO. It simply makes the code hard to test.

Whenever a library instantiates its own non-trivial objects in constructors and functions, it's usually only a matter of time until users of that library run into problems because they are unable to control the existence, properties and behavior of those objects.

And I can see downsides to making Date unmockable for timers - not only does it prevent the overriding of the behavior of timers (which is a bad idea but still), it also surprises the programmer using timers and Timecop. It's a very unexpected behavior, to be resilient against monkey-patching, I think.

Magnus Wolffelt

Oh and I would add that setTimeout is a function that, for testing purposes, normally should also be injected into code and be mocked by an immediateTimeout kind of function in tests.

Austin

I do agree with you that these things should happen a lot of the time but I am not a fan of a framework as broad as node forcing a specific architecture especially when it is not consistant with browser JS. It may be better practice it may just work better for you, I still think it is rather silly to brush of a change for stability and constancy just because your opinion of how software should be architected.

Magnus Wolffelt

These are just my opinions, I'm only pitching in and giving my view. I am in no way involved with development of Node. I think what makes me against this change is that it causes unexpected behaviour in Node code - as a monkey-patched Date implementation will be sort of application-wide, but not quite. It would be weird I think. Is there not a better solution?

James Alexander Rosen

@bnoordhuis I'm quite happy to fix whatever needs fixing in Timecop to make it work on node. As far as I can tell, though, there's no way for me to override this behavior from that library. (I could try to redefine setTimeout et al, but other libraries will already have a reference to the default implementation.)

Nathan Rajlich
Collaborator

I'm rather on the fence with this (leaning towards a -1 though)... on one hand, there's no harm in being resilient against the environment we're running in. But on the other hand anyone who overwrites the global Date object is asking for trouble, and I also feel that this solution is somewhat "hacky".

Additionally, there's probably other places in node-core where overwriting one of the built-in natives would cause problems, so this doesn't really seem like something we should be worrying about (or if we are, then to make sure to catch all the places where that can happen, which also sounds difficult).

James Alexander Rosen

@magwo said, "Code that wants to be testable, and relies on dates, timespans and such, should get all its time-related variables as parameters to constructors and functions."

That seems like you're saying tests should always determine the API. Let's say you're building a scheduling library, and you want to add an inNHours function. In a non-test-driven environment, you might write

Scheduler.prototype.inNHours = function(n, fn) {
  var at = +(new Date()) + ( n * 1000 * 60 * 60 );
  this.schedule(at, fn);
};

You want to test that behavior in an integration test, but you don't want to wait 3 hours for the tests to run. According to your rule, the solution would be to change the API to

Schedule.prototype.inNHours = function(n, fn, from) {
  from = from || new Date(),
  var at = +from + ( n * 1000 * 60 * 60 );
  this.schedule(at, fn);
};

This adds complexity to the API and the implementation to make your tests happy. In general, I'm for driving the design with tests (or with some other well-specified client) since it makes for better APIs. Maybe I should trust TDD here. After all, who am I to say that clients of this hypothetical Schedule wouldn't want the third argument? I usually like (small) dependency injection like this, so maybe I'm just hesitant because I wrote the library and want it to live on.

James Alexander Rosen

@TooTallNate It's certainly possible that Timecop, which has proven valuable in the Ruby community, simply doesn't fit in Node. The languages are substantially different. Perhaps most notably, it's common in JavaScript to save off a reference to an external object (say, Date). This is much less common in Ruby. Perhaps this one small (but important!) detail makes all the difference.

Isaac Z. Schlueter
Collaborator

How does this patch affect benchmarks? If it makes {set,clear}{Timeout,Interval,Immediate} faster, I'd take it. Otherwise, no.

Austin

@jamesarosen I have written the patches for timecop but never pushed them to you because of this oddity in node. :-1: . I agree there is a difference betten the ruby and node community but I still think "being resilient against the environment we're running in" is totally true.

As for the solution being hacky... I am not sure how, this is the point of closure, if date were to be passed in just as everyone is arguing the same problem would be solved, rather than relying on a global to exist.

Trevor Norris
Collaborator

Reminder to self. I'll throw up the benchmark numbers in the next week or so. Please remind me if I forget. =)

Nodejs Jenkins
Collaborator

Can one of the admins verify this patch?

Timothy J Fontaine tjfontaine closed this February 18, 2014
Timothy J Fontaine
Owner

We no longer use Date in timers.js so this shouldn't be a problem anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
3  lib/timers.js
@@ -26,6 +26,9 @@ var assert = require('assert').ok;
26 26
 // Timeout values > TIMEOUT_MAX are set to 1.
27 27
 var TIMEOUT_MAX = 2147483647; // 2^31-1
28 28
 
  29
+//Hold reference to the global date so if stubbed the timers still work
  30
+var Date = global.Date;
  31
+
29 32
 var debug;
30 33
 if (process.env.NODE_DEBUG && /timer/.test(process.env.NODE_DEBUG)) {
31 34
   debug = function() { require('util').error.apply(this, arguments); };
9  test/pummel/test-timers.js
@@ -44,6 +44,15 @@ setTimeout(function() {
44 44
   setTimeout_called = true;
45 45
 }, 1000);
46 46
 
  47
+assert.doesNotThrow(function() {
  48
+  var copy = Date;
  49
+  Date = 'Not a Date';
  50
+  interval_count = 0;
  51
+  setInterval(function() { clearInterval(this); }, 100);
  52
+  setTimeout(function() { return true; }, 100);
  53
+  Date = copy;
  54
+}, TypeError);
  55
+
47 56
 // this timer shouldn't execute
48 57
 var id = setTimeout(function() { assert.equal(true, false); }, 500);
49 58
 clearTimeout(id);
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.