skipUntil: fixes and tests

[staltz]

Resolves issue #577.

[trxcllnt]

Shouldn't the destination still be passed to the constructor?

[staltz]

If I do that, how should I write this class to actually unsubscribe when unsubscribe() is called?

[benlesh]

@staltz there's a bit of secret sauce in that constructor that is used for setting up the unsubscription chain. I'm having a hard time just from the code change discerning what moving away from super(destination) is fixing. Can you describe the issue in more detail?

[staltz]

Sure.

So, first of all I don't fully understand why some things were built as they are in Subscriber with regards to the shared subscriptions. Would be good if someone can clarify what was the intent.

If super(destination) is used, the secret sauce in the constructor will set this._subscription.
Later, in Subscriber.proto.unsubscribe() it does:

if (this._isUnsubscribed) {
  return;
} else if (this._subscription) {
  this._isUnsubscribed = true;
} else {
  super.unsubscribe();
}

this._isUnsubscribed is false, so we go to the second if check, and this._subscription is truthy, so we just end up setting this._isUnsubscribed to true, but not actually unsubscribing the current Subscriber from the Observable.

So this test would not pass:

  it('should skip all element and does not complete when another observable never completes', function () {
    var e1 =   hot( '--a--b--c--d--e--|');
    var e1subs =    '^                !'; // <== unsubscribe should happen, but never does!
    var skip = cold('-');
    var skipSubs =  '^                !';
    var expected =  '-';

    expectObservable(e1.skipUntil(skip)).toBe(expected);
    expectSubscriptions(e1.subscriptions).toBe(e1subs);
    expectSubscriptions(skip.subscriptions).toBe(skipSubs);
  });

The inner subscription for the skip Observable is correctly unsubscribed when e1 completes, but the subscription to e1 is not unsubscribed when e1 completes. Because of how Subscriber.proto.unsubscribe() was coded as I explained above.

That's not the only test that fails, other 3 tests also fail in a similar way: the subscription to the source isn't unsubscribed when the source completes.

PS: you already merged some PRs where I used the same technique: https://github.com/ReactiveX/RxJS/pull/562/files#diff-0864900d001ae5c1b7070f19e5e13a2eR33.

[benlesh]

I see what you're saying. It makes me wonder what other operators may have this side-effect from the single composite subscription architecture. @trxcllnt, what do you think?

It seems to me like, at least for skipUntil we can assign the destination in the subscriber directly rather than going through the super call. (As is in this PR). It does concern me that this will somehow break the unsubscription chain in cases where there is more than one operator before skipUntil, though. I haven't put much thought into that.

[benlesh]

I think I'm going to mark this one as blocked until we have a chance to investigate the possible ramifications of the change... also #587 .

[benlesh]

Okay, @staltz ... I think this is just waiting for the same treatment you gave takeUntil in #587

[staltz]

I think this is just waiting for the same treatment you gave takeUntil in #587

I tried to do that, but it's not the same case, and didn't work. skipUntil is a different beast than takeUntil.

[benlesh]

Yeah, you're right. I don't have time to investigate right now, but I have concerns about how this change will affect the subscription chain. It might be that in skipUntil it will have to be wired up more manually or something.

[benlesh]

@staltz @trxcllnt ... what do you think about leaving the super(destination) call in there, but overriding unsubscribe to behave slightly differently for this operator? I haven't investigated this, just sharing a thought.

[staltz]

Yeah, that come to my mind as well. I'm going to try it now.

[staltz]

@Blesh check if the latest commit does the trick.

[benlesh]

@staltz ... yeah. That's more in-line with what I was thinking. Perfect. Thanks, staltz.

[benlesh]

Can you, by chance, rebase this, @staltz?

[staltz]

Yep