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 August 13, 2021 14:41
repeat?: false
): Subscription;

export function executeSchedule(
Copy link
Member Author

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

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 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 August 13, 2021 14:44
@kwonoj
Copy link
Member

kwonoj commented Aug 13, 2021

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

@benlesh
Copy link
Member Author

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

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.

(Likely) Scheduled action leaks
3 participants