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.

Sign up for GitHub

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

Jump to bottom

refactor(shareReplay): avoid memory leaks #5554

Merged
merged 1 commit into from Aug 2, 2020
Merged

refactor(shareReplay): avoid memory leaks #5554

merged 1 commit into from Aug 2, 2020

Conversation

josepot
Copy link
Contributor

@josepot josepot commented Jul 2, 2020 •
edited

Description:
This PR is an attempt to address the following comment from @cartant on #5550

the problem the reverted PR caused exposed a separate issue that probably needs to be looked at: sources that complete synchronously won't end up with an undefined subscription and that wasn't the intention. In short, this reference-releasing behaviour needs more thought, so we've reverted it for now.

Related issue (if exists):
Somewhat related to this issue #5548

@josepot josepot force-pushed the chore/shareReplay-ensure-subscription-undefined branch from 229c58d to c21ec9b Compare July 2, 2020 07:57
@josepot josepot changed the title chore(shareReplay): ensure subscription is when source completes chore(shareReplay): ensure subscription is undefined after source completes Jul 2, 2020
@josepot josepot force-pushed the chore/shareReplay-ensure-subscription-undefined branch from c21ec9b to c1d5c37 Compare July 2, 2020 08:03
cartant
cartant requested changes Jul 3, 2020
View reviewed changes
src/internal/operators/shareReplay.ts Outdated Show resolved Hide resolved
@josepot josepot force-pushed the chore/shareReplay-ensure-subscription-undefined branch from c1d5c37 to 450c192 Compare July 3, 2020 13:09
@josepot josepot changed the title chore(shareReplay): ensure subscription is undefined after source completes refactor(shareReplay): avoid memory leaks Jul 3, 2020
@josepot josepot requested a review from cartant July 3, 2020 13:11
@josepot josepot force-pushed the chore/shareReplay-ensure-subscription-undefined branch from 450c192 to 4498642 Compare July 3, 2020 13:13
cartant
cartant requested changes Jul 26, 2020
View reviewed changes
Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

This looks right, to me, but the error callback could be simplified.

src/internal/operators/shareReplay.ts Outdated Show resolved Hide resolved
@josepot josepot force-pushed the chore/shareReplay-ensure-subscription-undefined branch from 4498642 to 3744fc9 Compare July 26, 2020 13:14
@josepot
Copy link
Contributor Author

josepot commented Jul 26, 2020

This looks right, to me, but the error callback could be simplified.

💯 done!

@josepot josepot requested a review from cartant July 28, 2020 19:22
cartant
cartant approved these changes Aug 1, 2020
View reviewed changes
Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM

@benlesh benlesh merged commit d9cfac3 into ReactiveX:master Aug 2, 2020
@josepot josepot deleted the chore/shareReplay-ensure-subscription-undefined branch August 3, 2020 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cartant cartant cartant approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@josepot @cartant @benlesh