Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: scheduling with Rx-provided schedulers will no longer leak action references #6562

Merged

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Aug 13, 2021

Resolves #6561

Note that I haven't quite figured out how I want to test this yet.

@benlesh benlesh requested review from cartant and kwonoj and removed request for cartant Aug 13, 2021
repeat?: false
): Subscription;

export function executeSchedule(
Copy link
Member Author

@benlesh benlesh Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic is here.

import { Subscription } from '../Subscription';
import { SchedulerAction, SchedulerLike } from '../types';

export function executeSchedule(
Copy link
Member Author

@benlesh benlesh Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added two type overloads for this internal function, to make sure we don't use this in a way that would leak with poorly written user-implemented schedulers.

@benlesh
Copy link
Member Author

@benlesh benlesh commented Aug 13, 2021

Good grief, I hate schedulers.

@benlesh benlesh force-pushed the fix-6561-unified-approach-to-fixing-leaks branch from 6526f14 to a579864 Compare Aug 13, 2021
@kwonoj
Copy link
Member

@kwonoj kwonoj commented Aug 13, 2021

Change looks ok, but looks like there's a new circular dependency?

@benlesh
Copy link
Member Author

@benlesh benlesh commented Aug 13, 2021

@kwonoj I've eliminated the circular dependencies as well as eliminating some more code that didn't really make sense.

The circular dependency was eliminated by moving innerFrom to its own file. But there was then another circular dependency in the fromArray.ts, which was only used internally in a few spots to basically do what from(arg, scheduler?) does but just for arrays. Eliminating a conditional was a silly optimization, so I got rid of internalFromArray and therefor fromArray.ts

kwonoj
kwonoj approved these changes Aug 14, 2021
@benlesh benlesh merged commit ff5a748 into ReactiveX:master Aug 14, 2021
5 checks passed
paulmojicatech pushed a commit to paulmojicatech/rxjs that referenced this issue Aug 17, 2021
…n references (ReactiveX#6562)

* fix: scheduling with Rx-provided schedulers will no longer leak action references

Resolves ReactiveX#6561

* refactor: Remove circular dependencies and redundant code

* chore: update api_guardian files
paulmojicatech pushed a commit to paulmojicatech/rxjs that referenced this issue Aug 17, 2021
…n references (ReactiveX#6562)

* fix: scheduling with Rx-provided schedulers will no longer leak action references

Resolves ReactiveX#6561

* refactor: Remove circular dependencies and redundant code

* chore: update api_guardian files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants