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(cache): get correct caching behavior #1765

Merged
merged 1 commit into from
Jun 14, 2016
Merged

fix(cache): get correct caching behavior #1765

merged 1 commit into from
Jun 14, 2016

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Jun 13, 2016

This is an initial pass at fixing the cache operator. There is still a lot to do, but this is merge-worthy IMO. Ideally, the cache operator would use the lift mechanism. Also this is not well optimized as it is introducing a lot of closures. But it works, and that's the point for now.

fixes #1628

This is an initial pass at fixing the cache operator. There is still a lot to do. Ideally, the cache operator would use the lift mechanism. Also this is not well optimized as it is introducing a lot of closures. But it works, and that's the point for now.

fixes #1628
@benlesh
Copy link
Member Author

benlesh commented Jun 13, 2016

(finally) lol

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 96.361% when pulling fc1509c on blesh:cache2 into 0a6c4e8 on ReactiveX:master.

@benlesh
Copy link
Member Author

benlesh commented Jun 14, 2016

Merged after quick review from @kwonoj

@benlesh benlesh merged commit cb0b806 into ReactiveX:master Jun 14, 2016
@staltz
Copy link
Member

staltz commented Jun 14, 2016

Kind of sad that it's all "rx4", but you know that already.

@benlesh
Copy link
Member Author

benlesh commented Jun 14, 2016

@staltz, yeah, I'll iterate on it.

@trxcllnt
Copy link
Member

@Blesh Is the difference between this and source.multicast(() => new ReplaySubject()).refCount() that this only recycles the ReplaySubject when the source emits an error (as opposed to when the subscriber count drops to 0)?

To phrase it another way, it seems like this operator now permanently leaks memory until the source emits an error. If true, this troubles me greatly.

@benlesh
Copy link
Member Author

benlesh commented Jun 15, 2016

Where is the leak? Can it be demonstrated?

@trxcllnt
Copy link
Member

trxcllnt commented Jun 16, 2016

@Blesh

// push 1000000 numbers into cache's ReplaySubject
const source = Observable.range(0, 1000000).cache();
const subscription = source.take(100000).subscribe({
  complete() {
    console.log(`cache's ReplaySubject now has 100000 values`);
  }
});

// cache now has a reference to a ReplaySubject with 100000 values.
// in order to release the reference for GC, I have to set source to null,
// but I can't since it's is a const.

subscription.unsubscribe(); // <-- this should do it

@matthewwithanm
Copy link

Is there documentation anywhere about how cache() is supposed to work now?

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

incorrect behavior of cache()
5 participants