fix(observeOn): release action references on teardown

[kwonoj]

Description:

Looks like we regressed #2244 while refactoring.

This PR brings up theoritically same changes to old fixes to release references once action is dispatched.

I have verified manually via devtools, but not sure if there's good way to create test case for this. Previous test case was inspecting deep internals.

BREAKING CHANGE:

Related issue (if exists):

[benlesh]

@kwonoj I think the bug is somewhere else. The Action returned by the schedule call extends Subscription. Therefor, when the Action unsubscribe is called, it should remove itself from whatever Subscription it has been added to.

However, what I'm not seeing is where the Action should unsubscribe itself after it has successfully executed, and not rescheduled. 🤔

I'm going to take a look at this. But I think maybe this isn't the right solution.

Regardless, I'm really not a fan of our Scheduler architecture, and I'm going to be trying to think of ways to migrate us to something better soon.

[kwonoj]

However, what I'm not seeing is where the Action should unsubscribe itself after it has successfully executed, and not rescheduled

I think that's crux of this issue? afaik we don't have places action self-unsubscribes once it complets. This is essentially we did before, and refactoring removed it.

I'm going to take a look at this.

Fine for me.

Regardless, I'm really not a fan of our Scheduler architecture,

I think most of us are on the same page, for me mostly where we have schedulers & schedules actions.

[kwonoj]

@benlesh fyi, this is original fix we did around observeOn. (https://github.com/ReactiveX/rxjs/pull/2248/files#diff-3e90d4c179a0e06539c3b8d62f8b782c59630cdb4e65d44fd926ea2147145aa0R53)

[benlesh]

@kwonoj Okay, so digging into this.... There's some clean up I can do to the code but the reason we keep around the Actions is because of the design of rescheduling. Technically, you can reschedule an action Asynchronously because of closures therefor, we can't unsubscribe the Action and have it remove itself from any parent subscription simply because it executed.

Ultimately, I now believe that this form of rescheduling is a design flaw that necessitates memory leaks or extra clean up code that requires "deep knowledge" of RxJS internals. Honestly, the requirement of "deep knowledge" of scheduler behaviors is one of the reasons I strongly believe using schedulers, in general, is inadvisable.

I think long-term we need to move away from our current Scheduler architecture, and move towards something more simplified. In the short term, we can fix observeOn in a manner similar to what we're doing here, but any user-land implementations leveraging our Schedulers will have the same problems with no obvious solutions.

[kwonoj]

@benlesh

because of the design of rescheduling

Yup, that's pretty much what I shared in slack channel.

the requirement of "deep knowledge" of scheduler behaviors is one of the reasons I strongly believe using schedulers, in general, is inadvisable.

agreed.

I think long-term we need to move away from our current Scheduler architecture, and move towards something more simplified

agreed again.

but any user-land implementations leveraging our Schedulers will have the same problems with no obvious solutions

it is already happening anyway, isn't it? as long as someone uses scheduling, or even plain observeOn operator, they're hitting this.

In the short term

I agree with all of this including long term fix. For now, I'd like to suggest short term fix to resurrect what we did previously, or similar way - while techinically it is not desirable, it is not breaking changes for the observeOn operator itself at least, and this impacts userland like redux-observable directly. (for now, any user uses redux-observable hits this 100%).

[kwonoj]

Just clarifying, I'm not saying this PR as-is should be checked in. I'm fine with other short term solutions.

[benlesh]

I feel like these are welcome changes... but are they necessary in this PR?

[kwonoj]

I can spliot actually, it was due to I got annoyed editor actually failed to initiate eslint. Up to you - but this should be harmless for the fix itself & other editor experiences.

[benlesh]

@kwonoj please review, I've changed the approach just a little bit, and I've added detailed comments.

[kwonoj]

🚢

[benlesh]

Citing @kwonoj's 🚢 here: #6559 (comment) in lieu of an approval, I'll take it.