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

SkipUntil with empty sequence never completes #326

Closed
GeorgeTsiokos opened this issue Feb 13, 2017 · 8 comments
Closed

SkipUntil with empty sequence never completes #326

GeorgeTsiokos opened this issue Feb 13, 2017 · 8 comments

Comments

@GeorgeTsiokos
Copy link

var sequence = Observable.Empty<Unit>();
var actual = Observable.Range(1, 5).SkipUntil(sequence).ToEnumerable().ToArray();
var expected = Enumerable.Range(1, 5).ToArray();
Assert.IsTrue(actual.SequenceEqual(expected));

Expected: OnCompleted when the source sequence completed
Actual: Never completes

Appears to be related to ReactiveX/RxJava#4511

@RxDave
Copy link
Contributor

RxDave commented Feb 14, 2017

Perhaps you shouldn't observe OnCompleted in the output sequence when the "until" sequence completes; otherwise, it would be ambiguous. I.e., you couldn't distinguish whether the source sequence was an empty sequence or whether the "until" sequence was an empty sequence.

Instead, wouldn't it be reasonable for the output sequence to simply never generate any notifications? However, if the "until" sequence is empty, then SkipUntil should at least dispose of the underlying subscription to the source. I haven't checked whether it does this already. If it doesn't, then I'd agree there's a bug.

@akarnokd
Copy link
Collaborator

To clarify the behavior, in RxJava 1.x if the other sequence completes with onCompleted, the gate is never opened and skipUntil skips all onNext items but terminates if the upstream signals onError or onCompleted. In RxJava 2.x if the other sequence completes with onComplete, the gate is opened an skipUntil relays events. In both versions the terminal events from upstream are relayed no matter what the gate's status is.

@GeorgeTsiokos
Copy link
Author

Returns the elements from the source observable sequence only after the other observable sequence produces an element.
https://github.com/Reactive-Extensions/Rx.NET/blob/master/Rx.NET/Source/System.Reactive.Linq/Reactive/Linq/Observable.Multiple.cs#L1393

Returns:

An observable sequence containing the elements of the source sequence starting from the point the other sequence triggered propagation.

If source or other completes, my expectation is the returned sequence should complete. I don't understand why the new sequence would never complete when other completes without a value. By definition, if the other sequence never triggers propagation, then the resulting sequence will never return a value. I understand it would not be possible to determine if the source or other completing triggered the completion of the resulting sequence, but, the caller specified that they wanted to skip all values from source until other produced a value. If we're able to guarantee other will never produce a value, (by stream completion), then, the resulting sequence will never return a value, and, we should be able to complete it.

For callers who want never behavior, you can pass an other argument who Concat with Observable.Never.

I wasn't aware that RxJava opens the gate when other completes. I was unable to find any documentation indicating that the sequence completion was considered "emitting an item". I think the underlining problem is that we need language-independent operator standards that document all of the event states to eliminate this type of confusion and different behavior across implementations.
http://reactivex.io/documentation/operators/skipuntil.html

For callers who want sequence closure to open the gate, pass an other argument who calls Materialize.

@danielcweber
Copy link
Collaborator

@akarnokd In reviewing #551 I found that it retains the behaviour, which I find puzzling as well. I wrote a failing unit test for that. The desired behaviour should be discussed before I can merge #551.

@akarnokd
Copy link
Collaborator

I kept the current expected behavior by the unit tests. Changing it would count as a drastic behavior change and should have happened with 4.0.0 final.

@danielcweber
Copy link
Collaborator

Still, what could have been the rationale ?

@akarnokd
Copy link
Collaborator

akarnokd commented May 31, 2018

When you do a library the first time without external independent feedback, you'll glance over practicalities such as not leaving the stream hanging even though terminal events have been fired.

I don't see any value turning a flow with SkipUntil into a never ending one by letting it signal an OnCompleted.

In RxJava, we open the gate upon onNext or onComplete as well as complete the downstream even when the gate is still closed. This avoids obvious and hard-todebug leaks.

It's hard to debug because the system has a state and no events fly around; no breakpoints can get hit and thus it is hard to inspect the memory.

@akarnokd
Copy link
Collaborator

I think this is by design in Rx.NET. You can defensively add DefaultIfEmpty to produce at least one item if the until source is empty.

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

No branches or pull requests

6 participants