RxJS asapScheduler stops working in 7.5.1

[arobinson]

Describe the bug

While trying to upgrade from RxJS6 to RxJS7 I noticed that sometimes our observables were not being executed. After debugging I found that at least asapScheduler did not always run the passed in callback function.

Expected behavior

asapScheduler should always run

Reproduction code

See the stackblitz

See the instructions in the stackblitz:

// To reproduce the issue (rxjs @ 7.5.1):
// 1. load this in stackblitz
// 2. observe that the logs up to '))) n 4' are printed
// 3. make a change to this file, note that it does not work (nothing runs)
// 4. reload the stackblitz page, you will see it work again

// To have it always work (rxjs @ 6.6.7):
// 1. load this in stackblitz
// 2. in the "Dependencies" on the left, enter rxjs@6.6.7
// 2. observe that the logs up to '))) n 4' are printed
// 3. make a change to this file, note that it still works

In our own application, this stops working after ~135 calls to the asapScheduler. The same two calls were failing, but I could cause it to fail sooner if I used asapScheduler more frequently. I managed to get it to fail in stackblitz, but only while changing the stackblitz file. I'm not 100% sure this reproduces the issue in our own application, but the symptoms are the same, and in both 6.6.7 works

Reproduction URL

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

Version

7.5.1

Environment

7.5.1 is causing the issue, but 6.6.7 works as expected

Additional context

Experienced in our corporate Angular 13 application

[cartant]

AFAICT, this was introduced in 7.5.0 in #6674. So 7.4.x should be okay. I think I can see the a problem - although, I have no idea why StackBlitz is behaving the way it is.

[benlesh]

It appears that it's not incrementing the id that the action is using, so it goes to reuse it, and then that some that was added tells it to clear the scheduled action.

[cartant]

It's this line:

rxjs/src/internal/scheduler/AsapAction.ts

Line 35 in 6d011f0

scheduler._scheduled = undefined;

_scheduled cannot be set to undefined unless it's equal to id. That's what's preventing further actions from being executed in this issue. However, looking at the source, I'm not sure that's the only problem. I'm going to create a failing test for this situation and will then open a PR to fix it. And I'll have more of a think about what I suspect are additional issues with the current implementation.

[backbone87]

can confirm this problem. cost me an entire day to narrow it down the asap scheduler. (and in the end I really wanted to use the queue scheduler anyway lol)

[cyberbiont]

Confirmed. My tests that use asapScheduler broke after I upgraded to the latest Rxjs version.
Downgrade to 7.4.0 solved it

[Harpush]

Ane news about that? Ngrx component store debounce use it... With this bug the debounced selectors stop working from time to time which means it's a blocker. We can't upgrade above 7.4.x with this bug

[aldrashan]

Ane news about that? Ngrx component store debounce use it... With this bug the debounced selectors stop working from time to time which means it's a blocker. We can't upgrade above 7.4.x with this bug

Same here. Can't upgrade to Angular 14 because @ngrx/component-store stops working correctly.

[benlesh]

There’s an apparent memory leak in the AsapScheduler, I’ve altered the example above to show it. If you keep updating the code, you’ll see the behavior in console. Settled actions aren’t removing themselves. When the scheduler loops through to see what it should execute, it’s probably tripping over old actions when checking against flushId or whatever in the code. I’ll try to have a look soon.

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

[aldrashan]

Any news on this?
Would be nice to get it fixed before Angular 15 comes out...

[josepot]

Hi @aldrashan ! I'm pretty sure that this issue got fixed on version 7.5.7. More specifically, I think that this issue got solved by this commit.

[jakovljevic-mladen]

Hey @aldrashan and @josepot.

More specifically, I think that this issue got solved by this commit.

I'm sorry, but the mentioned commit didn't fix this issue. The commit message mentioned that it was related to this issue, but it wasn't a fix. The change log generator generated an inaccurate message which Ben fixed with this commit - it most probably went unseen. Sorry about this.

[aokrushk]

It appears that 7.5.7 fixed something - at least the repo Stackblitz is working fine now.

[lorenzodallavecchia]

I think I am seeing the same issue, with 7.5.7 as well.
My application was hanging for no reason: took me two full days to pin down the problem in asapScheduler.

Please see this StackBlitz:
https://stackblitz.com/edit/rxjs-lpnmly?file=index.ts

In this case, we are calling unsubscribe on the scheduler subscription after it is triggered and after more actions (at least two) have been enqueued for the next tick. In this case, the flushId is getting misaligned with the enqueued actions. So, each time the scheduler fires, only the first action in the queue is executed and all the remaining ones are left there because they have different ids. If each action keeps scheduling more actions, this may easily cause a memory leak too.

As @cartant pointed out, the issue seems to be in the line that clears scheduler._scheduled.

The problem was introduced in 7.5.0. The last working version is 7.4.0.

NOTE - If you let StackBlitz auto-refresh the console when editing it will preserve the asapScheduler state from the previous attempts, possibly giving false results. In my example, I put a timestamp to distinguish actions firing from the previous attempts from actions scheduled in the current attempt. Use the manual reload button of StackBlitz to consistently reproduce the faulty behavior. This might also apply to the other test cases posted here.

[lorenzodallavecchia]

Update: calling unsubscribe is not necessary at all.
The problem is triggered by the act of scheduling two or more new actions during the execution of the first one.

See the updated StackBlitz:
https://stackblitz.com/edit/rxjs-khv49a?file=index.ts

[aldrashan]

Any chance this got fixed yet?
Would be a nice Christmas present.

[jonnyleeharris]

As a workaround I have created a custom scheduler based on #6889

import {SchedulerAction} from "rxjs"
import {immediateProvider} from "rxjs/internal/scheduler/immediateProvider"
import {AsapScheduler} from "rxjs/internal/scheduler/AsapScheduler"
import {AsyncAction} from "rxjs/internal/scheduler/AsyncAction"

// see https://github.com/ReactiveX/rxjs/pull/6889
// fix: animationFrameScheduler and asapScheduler no longer executing actions #6889
export class CustomAsapAction<T> extends AsyncAction<T> {
	constructor(protected scheduler: AsapScheduler, protected work: (this: SchedulerAction<T>, state?: T) => void) {
		super(scheduler, work);
	}
	protected requestAsyncId(scheduler: AsapScheduler, id?: any, delay: number = 0): any {
		// If delay is greater than 0, request as an async action.
		if (delay !== null && delay > 0) {
			return super.requestAsyncId(scheduler, id, delay);
		}
		// Push the action to the end of the scheduler queue.
		scheduler.actions.push(this);
		// If a microtask has already been scheduled, don't schedule another
		// one. If a microtask hasn't been scheduled yet, schedule one now. Return
		// the current scheduled microtask id.
		// @ts-ignore
		return scheduler._scheduled || (scheduler._scheduled = immediateProvider.setImmediate(scheduler.flush.bind(scheduler, undefined)));
	}
	protected recycleAsyncId(scheduler: AsapScheduler, id?: any, delay: number = 0): any {
		// If delay exists and is greater than 0, or if the delay is null (the
		// action wasn't rescheduled) but was originally scheduled as an async
		// action, then recycle as an async action.
		if ((delay != null && delay > 0) || (delay == null && this.delay > 0)) {
			return super.recycleAsyncId(scheduler, id, delay);
		}
		// If the scheduler queue has no remaining actions for the next schedule,
		// cancel the requested microtask and set the scheduled flag to undefined
		// so the next AsapAction will request its own.
		// @ts-ignore
		if (scheduler._scheduled === id && !scheduler.actions.some((action) => action.id === id)) {
			immediateProvider.clearImmediate(id);
			// @ts-ignore
			scheduler._scheduled = undefined;
		}

		// Return undefined so the action knows to request a new async id if it's rescheduled.
		return undefined;
	}
}

and then wherever I would use asapScheduler I replaced with new AsapScheduler(CustomAsapAction):

import {AsapScheduler} from "rxjs/internal/scheduler/AsapScheduler"

scheduled(new Subject(), new AsapScheduler(CustomAsapAction))

[aldrashan]

As a workaround I have created a custom scheduler based on #6889

import {SchedulerAction} from "rxjs"
import {immediateProvider} from "rxjs/internal/scheduler/immediateProvider"
import {AsapScheduler} from "rxjs/internal/scheduler/AsapScheduler"
import {AsyncAction} from "rxjs/internal/scheduler/AsyncAction"

// see https://github.com/ReactiveX/rxjs/pull/6889
// fix: animationFrameScheduler and asapScheduler no longer executing actions #6889
export class CustomAsapAction<T> extends AsyncAction<T> {
	constructor(protected scheduler: AsapScheduler, protected work: (this: SchedulerAction<T>, state?: T) => void) {
		super(scheduler, work);
	}
	protected requestAsyncId(scheduler: AsapScheduler, id?: any, delay: number = 0): any {
		// If delay is greater than 0, request as an async action.
		if (delay !== null && delay > 0) {
			return super.requestAsyncId(scheduler, id, delay);
		}
		// Push the action to the end of the scheduler queue.
		scheduler.actions.push(this);
		// If a microtask has already been scheduled, don't schedule another
		// one. If a microtask hasn't been scheduled yet, schedule one now. Return
		// the current scheduled microtask id.
		// @ts-ignore
		return scheduler._scheduled || (scheduler._scheduled = immediateProvider.setImmediate(scheduler.flush.bind(scheduler, undefined)));
	}
	protected recycleAsyncId(scheduler: AsapScheduler, id?: any, delay: number = 0): any {
		// If delay exists and is greater than 0, or if the delay is null (the
		// action wasn't rescheduled) but was originally scheduled as an async
		// action, then recycle as an async action.
		if ((delay != null && delay > 0) || (delay == null && this.delay > 0)) {
			return super.recycleAsyncId(scheduler, id, delay);
		}
		// If the scheduler queue has no remaining actions for the next schedule,
		// cancel the requested microtask and set the scheduled flag to undefined
		// so the next AsapAction will request its own.
		// @ts-ignore
		if (scheduler._scheduled === id && !scheduler.actions.some((action) => action.id === id)) {
			immediateProvider.clearImmediate(id);
			// @ts-ignore
			scheduler._scheduled = undefined;
		}

		// Return undefined so the action knows to request a new async id if it's rescheduled.
		return undefined;
	}
}

and then wherever I would use asapScheduler I replaced with new AsapScheduler(CustomAsapAction):

import {AsapScheduler} from "rxjs/internal/scheduler/AsapScheduler"

scheduled(new Subject(), new AsapScheduler(CustomAsapAction))

Fair enough, but we don't use AsapScheduler directly. It's being used in another library we have zero control over.

[arobinson]

We removed the asapScheduler in our code as well with a normal Promise implementation to get the same behavior. It appears that rxjs is not well maintained at all. Almost a year without any support.

[benlesh]

This should be fixed as of rxjs@7.5.7, please update.

[aldrashan]

This should be fixed as of rxjs@7.5.7, please update.

Still doesn't work for our project after updating to 7.5.7.
This issue should not be closed.

[aldrashan]

Not sure how helpful this is:

In our case, some actions cause the AsapScheduler to flush and others don't (i.e. actions get added to the actions array, but never get automatically flushed out via their id).
In the screenshot, the scheduler is handling actions with an id that's greater than the first action in the "actions" array.
This is fine when the action is passed as an argument to the flush function, but I guess it's not when it's just looping over the remaining actions in the actions array, since it'll only process the first action in the actions array and then stop (for each flush call)?
This way, there's gonna be a bunch of AsapActions stuck in the actions array (let's call it the queue), which will only get processed the next time flush is called without an action argument.

[lorenzodallavecchia]

This should be fixed as of rxjs@7.5.7, please update.

Still not working on 7.5.7, please see this StackBlitz: https://stackblitz.com/edit/rxjs-khv49a?file=index.ts
If you open the console, you can see that the scheduler loses the third task.

[WiseBird]

@benlesh Please reopen

[arobinson]

Can this please be addressed? It is a blocking bug and it is not fixed in any rxjs version. It has been broken for a year now with no one attempting to resolve the issue thus far.

[kwonoj]

We'll discuss this in next core team meeting again to confirm.

[kwonoj]

/cc @benlesh .

[benlesh]

CORE TEAM: Needs to be fixed in v7 and v8. We'll likely be introducing new/better/smaller schedulers for v9.

[matthewjh]

We are hitting the same issue, even with the latest 8 alpha at the time of writing, and this is blocking us from upgrading. don't really understand how this wasn't caught either through rx.js's tests or by piggybacking on other codebases to "e2e" test new rx.js versions.

[benlesh]

Okay... so here's the thing... THE ORIGINAL ISSUE that was filed here is indeed fixed as of 7.5.7.

There's a separate issue that is documented here: #6747 (comment)

I'm going to open a new Issue for that, and close this one to prevent confusion.

A friendly reminder to some of the snark in here: This is free software maintained by unpaid volunteers. I understand you're frustrated, but I'm quite literally not paid to deal with you.