Skip to content
This repository was archived by the owner on Feb 26, 2024. It is now read-only.

Ensure FakeAsyncTestZoneSpec tick always doTick #1099

Merged
merged 1 commit into from
Jul 27, 2018

Conversation

mitchellwills
Copy link
Contributor

In the case where there is pending work in the scheduler queue, but the duration
of the tick did not causes it to run the doTick callback would not be called (or
would not be called with intervals summing to the total time ellapsed).

this._currentTime = finalTime;
if (doTick) {
Copy link
Collaborator

@JiaLiPassion JiaLiPassion Jun 11, 2018

Choose a reason for hiding this comment

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

If the code is added here, the doTick may be triggered twice.
in your test case, when setTimeout callback is triggered, the doTick will be called twice.
consider the following case.

it('should run doTick callback even if no work ran', () => {
      fakeAsyncTestZone.run(() => {
        let log: number[] = [];
        function doTick(elapsed: number) {
           log.push(elapsed);
        }
        setTimeout(() => {}, 10);

        testZoneSpec.tick(6, doTick);
        testZoneSpec.tick(6, doTick);
        testZoneSpec.tick(6, doTick);
      });
    });

the log will be [6, 4, 2, 6] which I think will be correct.
But if the setTimeout delay is 12.
the log will be [6. 6. 0, 6]. I think we should not trigger doTick with this 0.

So we should add some logic to avoid this, such as

 tick(millis: number = 0, doTick?: (elapsed: number) => void): void {
      let finalTime = this._currentTime + millis;
      let lastCurrentTime = 0;
      if (this._schedulerQueue.length === 0 && doTick) {
        doTick(millis);
        return;
      }
      let queueConsumed = false;
      while (this._schedulerQueue.length > 0) {
        let current = this._schedulerQueue[0];
        if (finalTime < current.endTime) {
          // Done processing the queue since it's sorted by endTime.
          break;
        } else {
          // Time to run scheduled function. Remove it from the head of queue.
          queueConsumed = true;
          let current = this._schedulerQueue.shift();
          lastCurrentTime = this._currentTime;
          this._currentTime = current.endTime;
          if (doTick) {
            doTick(this._currentTime - lastCurrentTime);
          }
          let retval = current.func.apply(global, current.args);
          if (!retval) {
            // Uncaught exception in the current scheduled function. Stop processing the queue.
            break;
          }
        }
      }
      lastCurrentTime = this._currentTime;
      this._currentTime = finalTime;
      // if some task in queue have been invoked and the invoked task endTime equals
      // finalTime, we should not doTick with a zero again.
      if (!(queueConsumed && this._currentTime === lastCurrentTime) && doTick) {
        doTick(this._currentTime - lastCurrentTime);
      }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doTick will already be called with 0 today when there are multiple functions scheduled for execution at the same end time since doTick will always be called every time a scheduled function is invoked. Generally it's very unclear what the behavior of this callback should be since I couldn't find any existing tests for it.

I'm also not sure what queueConsumed in your snipit is for. Regardless of what scheduled work ran or not this function should always make sure it ticks any remaining time. And if you believe that doTick should not be invoked with 0 then the this._currentTime === lastCurrentTime check should be enough.

Copy link
Collaborator

@JiaLiPassion JiaLiPassion Jun 11, 2018

Choose a reason for hiding this comment

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

doTick will already be called with 0 today when there are multiple functions scheduled for execution at the same end time since doTick will always be called every time a scheduled function is invoked.

This is a little different, because in this case, doTick is triggered because the scheduled function is invoked. But consider the doTick name, it should mean fakeTimer ticked, so if several scheduled task endTime are the same, we should only doTick once.

queueConsumed just means if the doTick is triggered once by the scheduled function, and the finalTime equals to currentTime, we should not doTick again, but if doTick is not triggered by schedule function, even finalTime equals to currentTime, we still need to call doTick.

Generally it's very unclear what the behavior of this callback should be since I couldn't find any existing tests for it.

Yeah, this is old code, I am not sure when we should call doTick either, I will clarify the requirement and let's discuss again.

Copy link
Collaborator

@JiaLiPassion JiaLiPassion Jun 12, 2018

Choose a reason for hiding this comment

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

@mitchellwills
The doTick callback is from here. #866
The purpose is to advance Jasmine.mockDate, and in current newest zone.js, jasmine.mockDate has been advanced automatically, so we don't need to do that already.

So I think you may have another use case to use doTick, could you share your use case?

And I think the requirement should be,
doTick should be called when fake timer advanced.

So

  1. multiple scheduled macroTasks with the same end time should not trigger multiple doTick.
  2. 0 should be triggered, but still only once.

The reason that we should only doTick once, because consider the following case.

setTimeout(function() {
}, 0);
setTimeout(function() {
}, 0);
tick(0, millis => console.log)

the result should be

0 // triggered by the 1st timeout

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My use case is also advancing the clock when integrating with some tooling we have.

My view on this was that either there are very clear semantics on when doTick fires or it should be considered an implementation detail in which case it doesn't matter if doTick is called as long as the durations add up to the correct amount and it's best to just keep the code as simple as possible. Are there any other use cases aside from advancing the clock.

A few options:

  • Fires after each macro task
  • Only called with a non-zero value
  • Once for each macro task time advance and then the duration from the last macro task to the end time, possibly 0 (I think this is what you suggested)
  • It's an implementation detail and the only guarantee is that it will be called to increment time the correct amount

Some other edge cases

setTimeout(function() {}, 10);
setTimeout(function() {}, 10);
tick(10, millis => console.log);
// should this print 10 or 10, 0

setTimeout(function() {}, 0);
setTimeout(function() {}, 0);
tick(10, millis => console.log);
// should this print 0, 10 or 10

My personal opinion would be whatever leads to the simplest code unless there are specific semantics that would be advantageous.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Only called with a non-zero value

I am not sure about this one, I think doTick should fire when setTimeout(() => {}, 0); tick().
But if you mean Fires after each macro task is a stronger rule, I think it will be ok.
About the sample code you provided,

setTimeout(function() {}, 10);
setTimeout(function() {}, 10);
tick(10, millis => console.log);
// should this print 10 or 10, 0
// I prefer 10 here.

setTimeout(function() {}, 0);
setTimeout(function() {}, 0);
tick(10, millis => console.log);
// should this print 0, 10 or 10
// I prefer 0, 10 here

So the rule will be

  1. macroTask will fire doTick regardless of the duration is zero or not.
  2. If not match the rule 1, no macroTasks triggered, then we should also trigger doTick regardless of the duration is zero or not.
  3. If rule 1 is matched, some macroTask trigger doTick already, and after the trigger, the last triggered macroTasks' endTime equals finalTime, we should not doTick again with a zero.
// rule 1
setTimeout(function() {}, 0);
tick(0, (millis) => console.log);
// should print 0

// rule 2 and rule3
setTimeout(function(){}, 10);
tick(0, (millis) => console.log);
// should print 0

tick(5, (millis) => console.log);
// should print 5

tick(10, (millis) => console.log);
// should print 10 (not 10, 0)

what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that would be fine; however, I haven't seen any justification for the additional complexity.
If there comes a time where having a specific behavior makes sense then it could be added, but until then is there any reason to overconstrain the behavior (more complex to maintain, harder to change in the future, etc)? As long at the function fulfills advancing a clock the correct amount of time it doesn't seem like there is any need to define the behavior further than that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand your point. I believe currently doTick was only used inside google for their tests, so your current PR logic (may cause multiple doTick call) may cause some breaking changes in their test cases, I think we need @vikerman to verify it.

So my complex version just want to reduce the risk to impact on google test cases, anyway, I think this one need to be reviewed by @vikerman .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM
@vikerman I did run a global presubmit with the google code base. There were only one failure, but I don't think they were because of the number of calls to doTick.

@JiaLiPassion
Copy link
Collaborator

@vikerman , could you help to review this one (especially about when to trigger doTick and whether we should trigger doTick multiple times when multiple tasks have the same end time), Thank you!

@JiaLiPassion
Copy link
Collaborator

@mitchellwills, some other PR was merged, could you rebase this one? thanks.

In the case where there is pending work in the scheduler queue, but the duration
of the tick did not causes it to run the doTick callback would not be called (or
would not be called with intervals summing to the total time ellapsed).
@mitchellwills
Copy link
Contributor Author

Rebased.

@mhevery mhevery merged commit 9c96904 into angular:master Jul 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants