fix: scheduler unsubscription scenarios #6674

cartant · 2021-11-13T22:37:58Z

Description:

This PR adds failing tests and fixes for the following scenarios:

In the AnimationFrameScheduler and the AsapScheduler, it was possible for actions scheduled within a flush call - i.e. scheduled by actions executed with said flush call - to be themselves executed within the same flush call. This would only occur if actions were unsubscribed, as that would 'move' the newly-scheduled actions to a lower index in the actions queue - rendering the count-based mechanism unreliable for determining which actions should be executed.
In the AnimationFrameAction and the AsapAction, the cancellation of the animation frame and the clearing of the microtask depended upon whether or not the actions queue was empty. However, that's insufficient, as the actions queue can contain actions that are to be executed in separate flush calls. To determine whether cancellation is required, it's necessary to check that the actions queue has no actions that share the same async id.

Note that if the animation frame or microtask mentioned in the second scenario is not cancelled, flush is called with an empty actions queue and an error is effected:

TypeError: Cannot read properties of undefined (reading 'execute')
    at AsapScheduler.flush (node_modules/rxjs/src/internal/scheduler/AsapScheduler.ts:19:8

Version 6 is going to have the same problem - this has been a long-standing issue that has been hard to reproduce - so we should open another PR after this is merged to cherry-pick the change into that version.

Related issue: #6672 and #4690 and #2697

Closes ReactiveX#6672

cartant · 2021-11-14T06:54:04Z

spec/schedulers/AsapScheduler-spec.ts

+      const sandbox = sinon.createSandbox();
+      const setImmediateSpy = sandbox.spy(immediateProvider, 'setImmediate');
+      const setSpy = sandbox.spy(intervalProvider, 'setInterval');
+      const clearSpy = sandbox.spy(intervalProvider, 'clearInterval');


sandbox wasn't used consistently throughout this file, so I added a commit - 0dd8f02 - to address this.

matthewjh · 2021-11-25T09:11:39Z

Thanks @cartant

benlesh · 2021-11-29T21:32:52Z

src/internal/scheduler/AsapScheduler.ts


    do {
      if ((error = action.execute(action.state, action.delay))) {
        break;
      }
-    } while (++index < count && (action = actions.shift()));
+    } while ((action = actions[0]) && action.id === flushId && actions.shift());


It really feels like we should clean up this loop, but for now I think this change is solid.

No arguments from me re: clean up.

benlesh

Nice work. I can't wait until we have an opportunity to really rework the schedulers in the future. Some of what we have going on in here is a bit tangled.

benlesh · 2021-11-29T22:04:29Z

@cartant FYI: This isn't trivial to cherry-pick into 6.x, mostly because the tests don't work. The actual runtime code port is pretty straight forward.

benlesh · 2021-11-29T22:13:19Z

@cartant here is my attempt at cherry-picking the commit over: https://github.com/ReactiveX/rxjs/pull/6693/files

I don't have strong feelings about this fix making 6.x or not, given it's not really been a huge issue until now, but it might be nice to land it. I don't really want to waste a lot of time refactoring our testing strategies for the 6.x branch, but that's where all the sticking points are right now.

cartant · 2021-12-03T23:24:05Z

I'll take a look at the cherry-pick PR. The main reason for suggesting that it be cherry picked is that this issue has been occurring since version 5 - #2697 and #4690 - but no one had offered a reproduction of the problem until recently.

…heduler + Changes the check for existing action ids to simply check the last action in the queue to see if its id matches. Previously we were doing an O(n) loop on each execution of an action to check to see if the scheduling id needed to be recycled. This is problematic in AsapScheduler and AnimationFrameScheduler, where we're not reusing an interval. Since AsapScheduler and AnimationFrameScheduler reuse the most recent action id until their scheduled microtask or animation frame fires, the last action in the actions queue array is all we really need to check (rather than checking them all with `some`). O(1) vs O(n). + Refactors a weird conditional gaff from `if ((X && A) || (!X && B))` to just be `if (X ? A : B)` resolves ReactiveX#7017 related ReactiveX#7018 related ReactiveX#6674

…sapScheduler (#7059) * refactor(schedulers): Appropriately type action ids * fix(animationFrameScheduler): improve performance of animationFrameScheduler + Changes the check for existing action ids to simply check the last action in the queue to see if its id matches. Previously we were doing an O(n) loop on each execution of an action to check to see if the scheduling id needed to be recycled. This is problematic in AsapScheduler and AnimationFrameScheduler, where we're not reusing an interval. Since AsapScheduler and AnimationFrameScheduler reuse the most recent action id until their scheduled microtask or animation frame fires, the last action in the actions queue array is all we really need to check (rather than checking them all with `some`). O(1) vs O(n). + Refactors a weird conditional gaff from `if ((X && A) || (!X && B))` to just be `if (X ? A : B)` resolves #7017 related #7018 related #6674 * chore: update api_guardian * refactor(QueueAction): Have requestActionId return 0 Changes this to return `0` as a compromise given it was returning `void` in the past.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [rxjs](https://rxjs.dev) ([source](https://github.com/reactivex/rxjs)) | dependencies | patch | [`7.5.6` -> `7.5.7`](https://renovatebot.com/diffs/npm/rxjs/7.5.6/7.5.7) | --- ### Release Notes <details> <summary>reactivex/rxjs</summary> ### [`v7.5.7`](https://github.com/reactivex/rxjs/blob/HEAD/CHANGELOG.md#757-httpsgithubcomreactivexrxjscompare756757-2022-09-25) [Compare Source](ReactiveX/rxjs@7.5.6...7.5.7) ##### Bug Fixes - **schedulers:** improve performance of animationFrameScheduler and asapScheduler ([#7059](ReactiveX/rxjs#7059)) ([c93aa60](ReactiveX/rxjs@c93aa60)), closes [#7017](ReactiveX/rxjs#7017), related to [#7018](ReactiveX/rxjs#7018) and [#6674](ReactiveX/rxjs#6674) ##### Performance Improvements - **animationFrames:** uses fewer Subscription instances ([#7060](ReactiveX/rxjs#7060)) ([2d57b38](ReactiveX/rxjs@2d57b38)), closes [#7018](ReactiveX/rxjs#7018) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1562 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [rxjs](https://rxjs.dev) ([source](https://github.com/reactivex/rxjs)) | dependencies | minor | [`7.5.7` -> `7.8.0`](https://renovatebot.com/diffs/npm/rxjs/7.5.7/7.8.0) | --- ### Release Notes <details> <summary>reactivex/rxjs</summary> ### [`v7.8.0`](https://github.com/reactivex/rxjs/blob/HEAD/CHANGELOG.md#780-httpsgithubcomreactivexrxjscompare770780-2022-12-15) [Compare Source](ReactiveX/rxjs@7.7.0...7.8.0) ##### Features - **buffer:** `closingNotifier` now supports any `ObservableInput` ([#7073](ReactiveX/rxjs#7073)) ([61b877a](ReactiveX/rxjs@61b877a)) - **delayWhen:** `delayWhen`'s `delayDurationSelector` now supports any `ObservableInput` ([#7049](ReactiveX/rxjs#7049)) ([dfd95db](ReactiveX/rxjs@dfd95db)) - **sequenceEqual:** `compareTo` now supports any `ObservableInput` ([#7102](ReactiveX/rxjs#7102)) ([d501961](ReactiveX/rxjs@d501961)) - **share:** `ShareConfig` factory properties now supports any `ObservableInput` ([#7093](ReactiveX/rxjs#7093)) ([cc3995a](ReactiveX/rxjs@cc3995a)) - **skipUntil:** `notifier` now supports any `ObservableInput` ([#7091](ReactiveX/rxjs#7091)) ([60d6c40](ReactiveX/rxjs@60d6c40)) - **window:** `windowBoundaries` now supports any `ObservableInput` ([#7088](ReactiveX/rxjs#7088)) ([8c4347c](ReactiveX/rxjs@8c4347c)) ### [`v7.7.0`](https://github.com/reactivex/rxjs/blob/HEAD/CHANGELOG.md#770-httpsgithubcomreactivexrxjscompare760770-2022-12-15) [Compare Source](ReactiveX/rxjs@7.6.0...7.7.0) ##### Features - **distinct:** `flush` argument now supports any `ObservableInput` ([#7081](ReactiveX/rxjs#7081)) ([74c9ebd](ReactiveX/rxjs@74c9ebd)) - **repeatWhen:** `notifier` supports `ObservableInput` ([#7103](ReactiveX/rxjs#7103)) ([8f1b976](ReactiveX/rxjs@8f1b976)) - **retryWhen:** `notifier` now supports any `ObservableInput` ([#7105](ReactiveX/rxjs#7105)) ([794f806](ReactiveX/rxjs@794f806)) - **sample:** `notifier` now supports any `ObservableInput` ([#7104](ReactiveX/rxjs#7104)) ([b18c2eb](ReactiveX/rxjs@b18c2eb)) ### [`v7.6.0`](https://github.com/reactivex/rxjs/blob/HEAD/CHANGELOG.md#760-httpsgithubcomreactivexrxjscompare757760-2022-12-03) [Compare Source](ReactiveX/rxjs@7.5.7...7.6.0) ##### Bug Fixes - **schedulers:** no longer cause TypeScript build failures when Node types aren't included ([c1a07b7](ReactiveX/rxjs@c1a07b7)) - **types:** Improved subscribe and tap type overloads ([#6718](ReactiveX/rxjs#6718)) ([af1a9f4](ReactiveX/rxjs@af1a9f4)), closes [#6717](ReactiveX/rxjs#6717) ##### Features - **onErrorResumeNextWith:** renamed `onErrorResumeNext` and exported from the top level. (`onErrorResumeNext` operator is stil available, but deprecated) ([#6755](ReactiveX/rxjs#6755)) ([51e3b2c](ReactiveX/rxjs@51e3b2c)) #### [7.5.7](ReactiveX/rxjs@7.5.6...7.5.7) (2022-09-25) ##### Bug Fixes - **schedulers:** improve performance of animationFrameScheduler and asapScheduler ([#7059](ReactiveX/rxjs#7059)) ([c93aa60](ReactiveX/rxjs@c93aa60)), closes [#7017](ReactiveX/rxjs#7017), related to [#7018](ReactiveX/rxjs#7018) and [#6674](ReactiveX/rxjs#6674) ##### Performance Improvements - **animationFrames:** uses fewer Subscription instances ([#7060](ReactiveX/rxjs#7060)) ([2d57b38](ReactiveX/rxjs@2d57b38)), closes [#7018](ReactiveX/rxjs#7018) #### [7.5.6](ReactiveX/rxjs@7.5.5...7.5.6) (2022-07-11) ##### Bug Fixes - **share:** No longer results in a bad-state observable in an edge case where a synchronous source was shared and refCounted, and the result is subscribed to twice in a row synchronously. ([#7005](ReactiveX/rxjs#7005)) ([5d4c1d9](ReactiveX/rxjs@5d4c1d9)) - **share & connect:** `share` and `connect` no longer bundle scheduling code by default ([#6873](ReactiveX/rxjs#6873)) ([9948dc2](ReactiveX/rxjs@9948dc2)), closes [#6872](ReactiveX/rxjs#6872) - **exhaustAll:** Result will now complete properly when flattening all synchronous observables. ([#6911](ReactiveX/rxjs#6911)) ([3c1c6b8](ReactiveX/rxjs@3c1c6b8)), closes [#6910](ReactiveX/rxjs#6910) - **TypeScript:** Now compatible with TypeScript 4.6 type checks ([#6895](ReactiveX/rxjs#6895)) ([fce9aa1](ReactiveX/rxjs@fce9aa1)) #### [7.5.5](ReactiveX/rxjs@7.5.4...7.5.5) (2022-03-08) ##### Bug Fixes - **package:** add types to exports ([#6802](ReactiveX/rxjs#6802)) ([3750f75](ReactiveX/rxjs@3750f75)) - **package:** add `require` export condition ([#6821](ReactiveX/rxjs#6821)) ([c8955e4](ReactiveX/rxjs@c8955e4)) - **timeout:** no longer will timeout when receiving the first value synchronously ([#6865](ReactiveX/rxjs#6865)) ([2330c96](ReactiveX/rxjs@2330c96)), closes [#6862](ReactiveX/rxjs#6862) ##### Performance Improvements - Don't clone observers unless you have to ([#6842](ReactiveX/rxjs#6842)) ([3289d20](ReactiveX/rxjs@3289d20)) #### [7.5.4](ReactiveX/rxjs@7.5.3...7.5.4) (2022-02-09) ##### Performance Improvements - removed code that would `bind` functions passed with observers to `subscribe`. ([#6815](ReactiveX/rxjs#6815)) ([fb375a0](ReactiveX/rxjs@fb375a0)), closes [#6783](ReactiveX/rxjs#6783) #### [7.5.3](ReactiveX/rxjs@7.5.2...7.5.3) (2022-02-08) ##### Bug Fixes - **subscribe:** allow interop with Monio and other libraries that patch function bind ([0ab91eb](ReactiveX/rxjs@0ab91eb)), closes [#6783](ReactiveX/rxjs#6783) #### [7.5.2](ReactiveX/rxjs@7.5.1...7.5.2) (2022-01-11) ##### Bug Fixes - operators that ignore input values now use `unknown` rather than `any`, which should resolve issues with eslint no-unsafe-argument ([#6738](ReactiveX/rxjs#6738)) ([67cb317](ReactiveX/rxjs@67cb317)), closes [#6536](ReactiveX/rxjs#6536) - **ajax:** crossDomain flag deprecated and properly reported to consumers ([#6710](ReactiveX/rxjs#6710)) ([7fd0575](ReactiveX/rxjs@7fd0575)), closes [#6663](ReactiveX/rxjs#6663) #### [7.5.1](ReactiveX/rxjs@7.5.0...7.5.1) (2021-12-28) ##### Bug Fixes - export supporting interfaces from top-level `rxjs` site. ([#6733](ReactiveX/rxjs#6733)) ([299a1e1](ReactiveX/rxjs@299a1e1)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1668 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

cartant added 4 commits November 14, 2021 07:25

chore: use sinon sandbox consistently

0dd8f02

test: add failing flush tests

67e2c83

fix: don't execute actions scheduled within flush

fbaab9f

test: add failing tests

54688d4

cartant requested a review from benlesh November 13, 2021 22:38

cartant added 6.x Issues and PRs for version 6.x 7.x Issues and PRs for version 7.x labels Nov 13, 2021

fix: avoid calling flush with empty actions queue

fb9abf2

Closes ReactiveX#6672

cartant force-pushed the fix-6672 branch from b8c6c30 to fb9abf2 Compare November 13, 2021 22:39

cartant added 2 commits November 14, 2021 08:53

chore: remove accidental auto-import

1f31101

test: call all the dones

b5d0d17

cartant commented Nov 14, 2021

View reviewed changes

benlesh reviewed Nov 29, 2021

View reviewed changes

benlesh approved these changes Nov 29, 2021

View reviewed changes

benlesh added the 8.x Issues and PRs for version 8.x label Nov 29, 2021

benlesh merged commit e35f589 into ReactiveX:master Nov 29, 2021

cartant mentioned this pull request Jan 11, 2022

RxJS asapScheduler stops working in 7.5.1 #6747

Closed

benlesh mentioned this pull request Sep 8, 2022

fix(schedulers): improve performance of animationFrameScheduler and asapScheduler #7059

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: scheduler unsubscription scenarios #6674

fix: scheduler unsubscription scenarios #6674

cartant commented Nov 13, 2021 •

edited

Loading

cartant Nov 14, 2021

matthewjh commented Nov 25, 2021

benlesh Nov 29, 2021

cartant Nov 29, 2021

benlesh left a comment

benlesh commented Nov 29, 2021

benlesh commented Nov 29, 2021

cartant commented Dec 3, 2021

fix: scheduler unsubscription scenarios #6674

fix: scheduler unsubscription scenarios #6674

Conversation

cartant commented Nov 13, 2021 • edited Loading

cartant Nov 14, 2021

Choose a reason for hiding this comment

matthewjh commented Nov 25, 2021

benlesh Nov 29, 2021

Choose a reason for hiding this comment

cartant Nov 29, 2021

Choose a reason for hiding this comment

benlesh left a comment

Choose a reason for hiding this comment

benlesh commented Nov 29, 2021

benlesh commented Nov 29, 2021

cartant commented Dec 3, 2021

cartant commented Nov 13, 2021 •

edited

Loading