Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Remove code relating to parallel execution from PeriodicalExecuter. #23

Open
wants to merge 2 commits into from

3 participants

@arbales

In PeriodicalExcuter, removed unnecessary currentlyExecuting ivar and other code relating to parallel execution.

Since #onTimerEvent contained no logic after this change, made setInterval call (a bound) #execute and renamed #registerCallback to #start so that the timer can be restarted at will. This behavior already existed in the previous (undocumented) function.

Thanks!

@arbales arbales commented on the diff
test/unit/periodical_executer_test.js
@@ -12,24 +12,4 @@ new Test.Unit.Runner({
this.assertEqual(3, peEventCount);
});
},
-
- testOnTimerEventMethod: function() {
@arbales
arbales added a note

Didn't seem like this was performing any function that the previous test wasn't already doing, given that currentlyExecuting was removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arbales

(ping)

@arbales

Mind merging or closing?

@cecilphillip

I'm guessing they've given up over here

@arbales
@savetheclocktower
Collaborator

Sorry — we dropped the ball on pull requests for a while.

Can I ask why you think the code relating to parallel execution is unnecessary? I'm confused. It looks quite necessary to me if the point is to prevent an accumulation of callbacks in the case where your callback routinely takes longer than the interval. If your point is that JavaScript is single-threaded and thus nothing actually executes in "parallel," then I'll concede that it's a poor choice of words.

Just want to make sure I'm not missing something here.

@savetheclocktower
Collaborator

HAHAHA DISREGARD THAT (now I see your point)

Just shows how little this code has been touched over the years. Yeah, it doesn't prevent callback accumulation, and it's quite silly of us to have pretended that it did.

So I have four options, as I see it:

  1. Change the documentation to match existing behavior, but otherwise do nothing.
  2. Change the documentation to match existing behavior. Also, admit defeat on the issue and apply this patch, thereby conceding that PeriodicalExecuter is nothing more than a thin wrapper around setInterval.
  3. Change it to call setTimeout at the end of the callback execution, thus guaranteeing at least frequency seconds between calls (a somewhat-significant change in behavior).
  4. Change the behavior to match existing documentation. In other words, adopt an actual strategy for preventing callback accumulation. For instance, onTimerEvent could keep track of how long it's been since the last time it was called. Like, if we've got a frequency of one second, and onTimerEvent sees that it's been more than two seconds since the last time it was called, it could just return early, knowing that setInterval has scheduled another call to onTimerEvent that ought to run in no time flat.

I am leaning towards option #4 because we can't get rid of PeriodicalExecuter (backward compatibility), and as long as we're stuck with it I think it should serve some purpose beyond setInterval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 16, 2011
  1. @arbales

    In PeriodicalExcuter, removed unnecessary `currentlyExecuting` ivar a…

    arbales authored
    …nd other code relating to parallel execution.
  2. @arbales
This page is out of date. Refresh to see the latest.
View
31 src/prototype/lang/periodical_executer.js
@@ -31,13 +31,18 @@ var PeriodicalExecuter = Class.create({
initialize: function(callback, frequency) {
this.callback = callback;
this.frequency = frequency;
- this.currentlyExecuting = false;
- this.registerCallback();
+ this.start();
},
- registerCallback: function() {
- this.timer = setInterval(this.onTimerEvent.bind(this), this.frequency * 1000);
+ /**
+ * PeriodicalExecuter#start() -> undefined
+ *
+ * Starts the [[PeriodicalExecuter]] or restarts it if `#stop`
+ * was previously called.
+ **/
+ start: function() {
+ if (!this.timer) this.timer = setInterval(this.execute.bind(this), this.frequency * 1000);
},
execute: function() {
@@ -71,22 +76,4 @@ var PeriodicalExecuter = Class.create({
clearInterval(this.timer);
this.timer = null;
},
-
- onTimerEvent: function() {
- if (!this.currentlyExecuting) {
- // IE doesn't support `finally` statements unless all errors are caught.
- // We mimic the behaviour of `finally` statements by duplicating code
- // that would belong in it. First at the bottom of the `try` statement
- // (for errorless cases). Secondly, inside a `catch` statement which
- // rethrows any caught errors.
- try {
- this.currentlyExecuting = true;
- this.execute();
- this.currentlyExecuting = false;
- } catch(e) {
- this.currentlyExecuting = false;
- throw e;
- }
- }
- }
});
View
20 test/unit/periodical_executer_test.js
@@ -12,24 +12,4 @@ new Test.Unit.Runner({
this.assertEqual(3, peEventCount);
});
},
-
- testOnTimerEventMethod: function() {
@arbales
arbales added a note

Didn't seem like this was performing any function that the previous test wasn't already doing, given that currentlyExecuting was removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- var testcase = this,
- pe = {
- onTimerEvent: PeriodicalExecuter.prototype.onTimerEvent,
- execute: function() {
- testcase.assert(pe.currentlyExecuting);
- }
- };
-
- pe.onTimerEvent();
- this.assert(!pe.currentlyExecuting);
-
- pe.execute = function() {
- testcase.assert(pe.currentlyExecuting);
- throw new Error()
- }
- this.assertRaise('Error', pe.onTimerEvent.bind(pe));
- this.assert(!pe.currentlyExecuting);
- }
});
Something went wrong with that request. Please try again.