Skip to content
This repository has been archived by the owner on Dec 7, 2019. It is now read-only.

No way to clean up Store instance that cancels pending requests #287

Open
jasennett opened this issue Nov 17, 2017 · 4 comments
Open

No way to clean up Store instance that cancels pending requests #287

jasennett opened this issue Nov 17, 2017 · 4 comments

Comments

@jasennett
Copy link

If a store instance is no longer needed there's no way to tear it down without leaving any outstanding inflight requests running. Currently clear() invalidates inflight requests, but doesn't cancel the actual network request. Ideally there would be a way to destroy a store that also cancels those requests. Might be a little challenging to add because the rx cache() operator that the store is currently using doesn't provide a way to dispose the underlying subscription and I'm not sure if there is an equivalent operator that does allow that.

@digitalbuddha
Copy link
Contributor

Hey this one is a bit tricky, I'm not opposed to it just not sure how to implement currently. Open to suggestions :-)

@jasennett
Copy link
Author

Yeah, it is a bit tricky because of the cache() operator not providing a way to dispose the source subscription. Two potential ways to get around this that I can think of:

  1. Extend Rx's Single like SingleCache, but instead of ignoring the source disposable hold on to it and expose an interface to dispose it. I threw together this gist where I copied Rx's SingleCache and modified it to make it disposable. The store could change it's inflight request to be this DisposableSingleCache instead of a normal cached Single so they could then be disposed on clear. One potential downside to this approach is that this DisposableSingleCache interface isn't supported as part of Rx's plugin system so you skip the onAssembly plugin call that the cache() operator currently does. You could work around this by having the inflight request be a pair of the actual DisposableSingleCache instance and then call the plugin method to get a generic Single for consumption and just use the DisposableSingleCache instance for disposing in clear()

  2. Use a BehaviorSubject to cache your results instead of the cache() operator. Create your Single like normal, but instead of exposing it directly, subscribe the results to a BehaviorSubject and save your inflight request as a pair of the disposable from that subscription and the behavior subject itself. Then anywhere you would currently return the single you can just use singleOrError() on the behavior subject to convert it so the store interface doesn't have to change at all. One disadvantage of this approach would be that because you are immediately subscribing the single to the behavior subject your request would start immediately, even if the store's consumer hasn't subscribed to the Single result, so it's more or less turning it from a cold observable to a hot observable that could be wasting bandwidth if none of the store's consumers ever subscribe to the returned single.

I think I'd personally lean more towards the first approach, feels a bit cleaner to me, and whether or not you even bother with letting the rx plugins wrap the cached single feels like something you could take or leave. I'm sure there's other ways to approach this as well that might be better, but that's what I came up with. Thoughts?

@digitalbuddha
Copy link
Contributor

Great ideas! Let me digest over the weekend. In theory I like 2's simplicity but maybe we can do in a defer fashion. alternatively we can change the inflight architecture in general.

@charbgr
Copy link
Contributor

charbgr commented Apr 4, 2018

Do you have any update on this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants