Broken QueueScheduler (or TestScheduler)

[alex-okrushko]

Bug Report

Current Behavior

scheduled(observable$, queueScheduler) does not queue up values ( or TestScheduler is broken)

Expected behavior

scheduled(observable$, queueScheduler) queues up values. 😅

Reproduction

We are updating NgRx to RxJS v7 (ngrx/platform#3109) and a few tests started to fail. When bisecting the PRs it was found that 7.0.0-beta.4 works as expected, while 7.0.0-beta.5 and on are broken. git bisect further identified that #5619 is when issue started to happen.

In ComponentStore we rely on queueScheduler to make sure that all values are executed, and tests that verify it started to fail.

Please provide repro code does not involves framework, or other setups to address this bug is specific to rxjs core

Here are TWO min repro tests - one with scheduled(updater$, queueScheduler) that queues, and the other with just updater$ that doesn't does. With RxJS 7.0.0-beta.4 both are passing: https://stackblitz.com/edit/jasmine-testing-hv2emp?file=src%2Freducer.spec.ts

first test:

scheduled(updater$, queueScheduler).subscribe((v: string) => {
        subject$.next(v);
      });

vs second test:

updater$.subscribe(v => {
        subject$.next(v);
      });

Except for the "expected" results, the tests are identical.

Failing test

Now here are the same TWO repro tests, but in with RxJS 7.0.0-beta.5 (that includes #5619) and one of the tests is failing. That failing test is using scheduled(updater$, queueScheduler), however the output results are as if it's using just updater$ without scheduled and queueScheduler.
Here's the stackblitz: https://stackblitz.com/edit/jasmine-testing-snrg1g?file=src%2Freducer.spec.ts

Environment

• Runtime: node 14
• RxJS version: issue is still present in 7.3.0.

Possible Solution

In ##5619 both the TestScheduler and queueScheduler are adjusted, so it's hard to tell which one is actually broken. Needs further investigation.

[cartant]

I don't believe this is a bug. Prior to the PR you have referenced, it was not possible to properly test code that used the queueScheduler or the asapScheduler, as the core schedule implementation in AsyncScheduler:

rxjs/src/internal/scheduler/AsyncScheduler.ts

Lines 37 to 43 in 881cacd

	public schedule<T>(work: (this: SchedulerAction<T>, state?: T) => void, delay: number = 0, state?: T): Subscription {
	if (AsyncScheduler.delegate && AsyncScheduler.delegate !== this) {
	return AsyncScheduler.delegate.schedule(work, delay, state);
	} else {
	return super.schedule(work, delay, state);
	}
	}

was patched in the TestScheduler's run method:

rxjs/src/internal/testing/TestScheduler.ts

Lines 374 to 381 in 881cacd

	run<T>(callback: (helpers: RunHelpers) => T): T {
	const prevFrameTimeFactor = TestScheduler.frameTimeFactor;
	const prevMaxFrames = this.maxFrames;
	TestScheduler.frameTimeFactor = 1;
	this.maxFrames = Number.POSITIVE_INFINITY;
	this.runMode = true;
	AsyncScheduler.delegate = this;

That means that any scheduler used within run would behave like the TestScheduler version of the AsyncScheduler.

In the referenced PR, this patching was changed, so each scheduler's implementation is actually what's used in the tests and it's the APIs - like setInterval - that are patched instead. That means that the queueScheduler behaves that same way within TestScheduler#run as it behaves in an app: it only queues scheduled actions whilst a scheduled action is being executed.

AFAICT, nothing in your StackBlitz's 'queues values' test will schedule actions from within a scheduled action, so the use of queueScheduler will be ineffectual. Each scheduled action will finished executing before another is scheduled, so there will be no queuing:

rxjs/src/internal/scheduler/AsyncScheduler.ts

Lines 30 to 44 in 9a40869

	if (this._active) {
	actions.push(action);
	return;
	}
	let error: any;
	this._active = true;
	do {
	if ((error = action.execute(action.state, action.delay))) {
	break;
	}
	} while ((action = actions.shift()!)); // exhaust the scheduler queue
	this._active = false;

It looks to me like the 'queues values' test was written to test what's actually the asyncScheduler behaviour and if queueScheduler is replaced with asyncScheduler, the test will pass.

I would imagine that it makes sense to use the queueScheduler in your ComponentStore - so that changes that synchronously effect changes have the effected changes queued (I imagine that this is what you're after), but with the referenced PR merged, queueSchedulers in run tests really will behave like they're supposed to and not like an asyncScheduler with a duration of zero.

[alex-okrushko]

@cartant Thanks for the great explanation! 👍

I was assuming my tests were utilizing queue behaviour.

Closing this one.