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(shareReplay): shareReplay unsubscribe routine is called #3354

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@liqwid
Copy link

liqwid commented Feb 28, 2018

Unsubscribe routine for shareReplay was ingonred. Now it is being called.

Related issue (if exists):
Fixed while exploring #3336 however does not fix the issue, details in the issue comments.

fix(shareReplay): shareReplay unsubscribe routine is called upon unsu…
…bscribe

Unsubscribe routine for shareReplay was ingonred. Now it is being called.

@liqwid liqwid force-pushed the liqwid:shareReaplay-unsubscribe-fix branch from 8b6987b to acbb8e4 Mar 3, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 3, 2018

Coverage Status

Coverage increased (+0.06%) to 97.607% when pulling 372d7fd on liqwid:shareReaplay-unsubscribe-fix into 556c904 on ReactiveX:master.


return () => {
const _unsubscribe = this.unsubscribe;
this.unsubscribe = function() {

This comment has been minimized.

@benlesh

benlesh Mar 4, 2018

Member

I don't think that monkey-patching the destination subscriber is what we really want to do here.

This comment has been minimized.

@liqwid

liqwid Mar 4, 2018

Agreed, will try to refactor shareReplay as a class, like other operators.

liqwid added some commits Mar 4, 2018

fix(shareReplay): added removeSubscriber callback upon shareReplay un…
…subscription

ShareReplay subscribes with ShareReplaySubscriber that recieves an onUnsubscribe callback
removeSubscriber is passed to ShareReplaySubscriber as an onUnsubscribe callback
@benlesh
Copy link
Member

benlesh left a comment

Needs a test to verify the change is working.

@liqwid

This comment has been minimized.

Copy link

liqwid commented Mar 16, 2018

Actually any unsubscribing/refCount/isComplete logic is currently redundant in shareReplay
That's because currently unsubscribe logic waits for source to be completed and source unsubscribes itself upon completion.

@liqwid liqwid force-pushed the liqwid:shareReaplay-unsubscribe-fix branch from 34702f6 to 685bdc3 Mar 16, 2018

fix(shareReplay): removed shareReplay redundant unsubscribe logic
source unsubscribes itself upon completion so any other unsubscription logic is redundant

@liqwid liqwid force-pushed the liqwid:shareReaplay-unsubscribe-fix branch from 440729f to 2cd193b Mar 16, 2018

@benlesh

This comment has been minimized.

Copy link
Member

benlesh commented Mar 30, 2018

I think the big debate here with the core team, is we're unsure what the behavior should be in this situation. That's the real hold up with this PR.

@voithos

This comment has been minimized.

Copy link

voithos commented Apr 2, 2018

@benlesh, as a proposal, what about having the following unsubscribing behavior for shareReplay:

  • If refCount === 0, check if the source has completed.
    • If the source has not completed, unsubscribe from it and clean up the Subject. This avoids the problem mentioned in #3336.
    • If the source has completed, then don't remove the Subject - instead, reuse it upon future subscriptions. This partially avoids the problem mentioned in #2908 while still allowing cancellation.
  • If refCount !== 0, of course, just continue as normal.

(Let me know if you'd prefer the discussion happen on #3336)

@PSanetra

This comment has been minimized.

Copy link

PSanetra commented Apr 3, 2018

My thoughts:

  1. The current approach of this PR would break with the documentation of the share function. A different behavior between share and shareReplay would be unexpected.

[...] When all subscribers have unsubscribed it will unsubscribe from the source Observable. [...]

  1. I think you should fix Observable.subscribe since it does not respect the contract of the Operator interface. Operator.call may return a TeardownLogic, which should be handled by the caller.
@liqwid

This comment has been minimized.

Copy link

liqwid commented Apr 4, 2018

@voithos bringing refCount to 0 will still restart uncompleted source in the described case, which was the main point of the issue. However share does restart source upon full unsubscription also http://jsbin.com/mexuma/1/edit?js,output
@benlesh since share behaves the same way it looks to me that #2908 is not a bug, since share & shareReplay only retain the source hot until there are subscribers present.
To prevent the restart the source should be hot to start with, for example using publish and connect.
@PSanetra agreed on the TeardownLogic since operators are supposed to return it according to their type declaration.

@voithos

This comment has been minimized.

Copy link

voithos commented Apr 4, 2018

Indeed, share would not be able to have the behavior I described since it doesn't use a ReplaySubject or BehaviorSubject, and thus "reusing the Subject" wouldn't accomplish anything if the source Observable wasn't subscribed to.

If I understand correctly, the previous behavior of share and shareReplay was effectively equivalent to publish().refCount() and publishReplay(N).refCount(), respectively. If these are intended to be equivalent, then I agree that #2908 wasn't a bug.

Still, I think there might be a use case for the behavior I described. As an example, consider an Observable that represents an HTTP request. Ideally:

  • It would be cold until subscribed to, because there's no need to execute the expensive operation (in this case, an HTTP request) unless something needs the result. So we cannot use connect().
  • After completing, it would not re-subscribe to the source but instead would use something like a ReplaySubject(1) to retain the last value. Thus, publishReplay(1).refCount() or the old shareReplay(1) behavior isn't ideal because it re-subscribes every time the refCount goes from 0 -> 1.
  • If the source subscription gets cancelled before completion, then re-subscribing is ok since we never got a value anyway and thus can't use the underlying ReplaySubject.

If it doesn't make sense to have shareReplay do this, then perhaps a new operator might be useful? Unless I'm missing an important failure mode.

@benlesh

This comment has been minimized.

Copy link
Member

benlesh commented Jul 26, 2018

Given that this is a breaking change, and the demand for this change in behavior hasn't been especially high, I'm going to close this issue for now.

@liqwid, I really appreciate your effort here, and I know it sucks to have a PR closed. Hopefully that doesn't put you off from contributing again in the future!

Thank you again!

@benlesh benlesh closed this Jul 26, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 25, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.