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(delay): fix memory leak (in stable) #3667

Merged
merged 1 commit into from
May 21, 2018

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented May 8, 2018

Description:

This PR cherry picks PR #3605 into the stable branch. Given that it fixes a memory leak, I think there's something to be said for its application to v5, too.

Related issue (if exists): #3604

@cartant cartant changed the base branch from master to stable May 8, 2018 00:02
* test(delay): failing test

* fix(delay): fix memory leak
@rxjs-bot
Copy link

rxjs-bot commented May 8, 2018

Messages
📖

CJS: 2282.9KB, global: 751.3KB (gzipped: 120.8KB), min: 145.8KB (gzipped: 31.2KB)

Generated by 🚫 dangerJS

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0004%) to 97.402% when pulling 9d1ba14 on cartant:issue-3605-stable into 21a59a7 on ReactiveX:stable.

@benlesh
Copy link
Member

benlesh commented May 9, 2018

@cartant ... I'm torn on this, if we patch 5.x, we'll also need to patch 5.6.0-forward-compat. At some point we're going to need to move on (unless there's some glaring security issue).

@benlesh
Copy link
Member

benlesh commented May 9, 2018

cc @kwonoj

@benlesh benlesh added AGENDA ITEM Flagged for discussion at core team meetings type: discussion labels May 9, 2018
@cartant
Copy link
Collaborator Author

cartant commented May 10, 2018

@benlesh Yeah, I get that not all fixes should be backported, but memory leaks are especially insidious. And the delay operator is not uncommon.

I guess it depends upon how many apps using v5 would benefit from the fix. To benefit, they'd have to be using delay on a long-lived stream.

I don't have a strong opinion either way, but I'd favour backporting.

@benlesh benlesh merged commit 6b08b13 into ReactiveX:stable May 21, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jun 20, 2018
@cartant cartant deleted the issue-3605-stable branch September 24, 2020 07:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
AGENDA ITEM Flagged for discussion at core team meetings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants