Scheduler.flush throws unclear error when actions is empty (cannot read property 'execute' of undefined)

[notnarb]

Bug Report

Current Behavior
Calling .flush() on a scheduler (e.g. animationFrameScheduler) with no pending actions causes an unclear exception to be thrown:

TypeError: Cannot read property 'execute' of undefined
at AnimationFrameScheduler.flush (https://rxjs-pc1ga8.stackblitz.io/turbo_modules/[email protected]/internal/scheduler/AnimationFrameScheduler.js:31:32)
at Object.eval (https://rxjs-pc1ga8.stackblitz.io/~/index.ts:12:36)
at eval (https://rxjs-pc1ga8.stackblitz.io/~/index.ts:19:4)
at eval (https://rxjs-pc1ga8.stackblitz.io/~/index.ts:20:3)
at eval (<anonymous>)
at Qt (https://c.staticblitz.com/d/webcontainer.e1e926a8972546110c3.js:15:30145)
at https://c.staticblitz.com/d/webcontainer.e1e926a8972546110c3.js:15:38799
at U (https://c.staticblitz.com/d/webcontainer.e1e926a8972546110c3.js:15:13565)
at https://c.staticblitz.com/d/webcontainer.e1e926a8972546110c3.js:15:13207
at boot (https://rxjs-pc1ga8.stackblitz.io/dev/boot:6:3)

Reproduction

• REPL or Repo link: https://stackblitz.com/edit/rxjs-pc1ga8?devtoolsheight=60

import {animationFrameScheduler} from 'rxjs';
animationFrameScheduler.flush();

Expected behavior
Either one of:

• not throwing when there are no pending actions
• throwing a clearer error (e.g. 'Attempting to flush an empty queue')

Environment

• Runtime: Chrome v73
• RxJS version: 6.4.x

Possible Solution

See expected behavior. This appears to affect all schedulers that have this do while loop without verifying that a valid action has been shift'd off of this.actions

This may be working as intended (and no change desired for performance reasons) in which case just having this issue open and google-able would provide additional documentation

Additional context/Screenshots

#2697 Appears to have been the same issue given their stacktrace: https://github.com/ReactiveX/rxjs/blob/5.4.1/src/scheduler/AsapScheduler.ts#L17

A clearer error (or no error at all) would probably have pointed them in the right direction.

---

Happy to submit a fix myself if the desired behavior is defined.

[cartant]

Are you able to effect this error without an explicit call to flush? IMO, you should not be calling it. When to flush is up to the scheduler, not the application code.

[notnarb]

Are you able to effect this error without an explicit call to flush

Not that I am aware of. My specific use case is flushing async schedulers during tests. Otherwise I need to mock the behavior of Window.requestAnimationFrame in tests.

My current workaround is just to check the number of actions before calling requestAnimationFrame which works but is less than ideal.

[cartant]

I don't think this is a bug. I think you should be using the TestScheduler within tests. The run method that was added in v6 makes this much easier than it used to be.

You might also want to read this: https://ncjamieson.com/testing-with-fake-time/

[decafdennis]

Still working on steps to repro, but FWIW we have a significant volume of Bugsnag reports of this bug in production despite not calling flush() explicitly anywhere:

Edit: this is with [email protected], and not isolated to any browser.

[cartant]

I've had a quick look at this and I cannot reproduce the problem. And I cannot see any obvious flaw in the code. Note that cancelAnimationFrame is called here if the actions array is empty.

Without more context, there is nothing further that I can do. I note that version 6.4.x is mentioned in the issue description. However, there is a reference to version 5.4.1 source. And I have no idea of the version to which you are attributing the reported Bugsnag failures.

[notnarb]

I don't think this is a bug. I think you should be using the TestScheduler within tests. The run method that was added in v6 makes this much easier than it used to be.

Thanks for the heads up! I did not know about this scheduler and it seems really cool!

Unfortunately TestScheduler, as far as I can tell, will cause an infinite loop when running interval(0, animationFrameScheduler) because that becomes interval(0) which will eventually crash the browser.

https://stackblitz.com/edit/rxjs-kkntbc?file=index.ts

---

Whether or not there is a workaround to my specific problem with using TestScheduler, I still think it might be useful to prevent future issues if one of the following changes was made:

1. JsDoc be added to flush() specifying that it is a private API and should not be used externally
2. JsDoc be added to flush() specifying that it will throw if there are no pending actions
3. flush() does not throw if there are not any pending actions.

Any of which I would be more than happy to contribute.

[cartant]

You might be better off testing using fake time. Have a look at https://ncjamieson.com/testing-with-fake-time/

Regarding the documentation, it's TSDoc - not JSDoc - and @internal effects specific behaviour that, IIRC, is undesirable.

Regarding the public interface for schedulers, it's this:

https://rxjs.dev/api/index/interface/SchedulerLike

Anything not in that interface is an implementation detail.

[notnarb]

For what it's worth, Jasmine and Jest do not support faking requestAnimationFrame with their built-in fake time libraries. Sinon does, but switching to Sinon in order to take advantage of the animationFrameScheduler is not within scope of what I am hoping to accomplish.

I initially ran into some issues manually mocking requestAnimationFrame and having it work as expected with the animationFrameScheduler, but I will file a different bug for that when I next run into this issue.

---

I do not entirely agree that it is currently intuitive that SchedulerLike is the public API for animationFrameScheduler and I still think the code would benefit from some sort of clearer communication when the above error is thrown, but that's my own opinion. At the very least this issue will exist if someone else encounters this error.

Regardless, I appreciate your work on this project: thank you for helping to maintain rxjs!

I am fine with closing this issue (unless you wanted additional information from @decafdennis)

[cartant]

My concern is that the issue is not reproducible and that there are historical issues - AFAICT - in some browsers with cancelAnimationFrame not working. Without a repro, there is nothing that can be done, IMO.

Regarding testing, a polyfill is used in this project to allow the scheduler to be tested in Node. Such an approach should work fine with fake time without Sinon.