1.x: fix take(-1) not completing

[akarnokd]

Originally, only 0 was checked which resulted in onCompleted() but negative values weren't. When the downstream requested, c became -1 and was requested from the source. range ignores negative requests but other sources may throw IAE in that case.

With the fix, the operator will throw IAE in assembly time.

[zsxwing]

👍

[JakeWharton]

Why not IAE on negative values for the operator? This seems like it could hide errors in math-based take values.

[akarnokd]

I don't know, could be some historical reasons from the early days of RxJava.

[zsxwing]

Just checked Rx.Net and RxJS. Both them throw an error for negative values:

https://github.com/Reactive-Extensions/Rx.NET/blob/859e6159cb07be67fd36b18c2ae2b9a62979cb6d/Rx.NET/Source/System.Reactive.Linq/Reactive/Linq/Observable.StandardSequenceOperators.cs#L1279
https://github.com/Reactive-Extensions/RxJS/blob/master/src/modular/observable/take.js#L47

So I suggest let's throw IAE.

[zsxwing]

Could you also check skip?

[akarnokd]

skip(0) and skip(-1) just returns the source without applying the operator to it.

For 2.x, definitely, but I'm not certain about 1.x, sounds like an API change.

[zsxwing]

cc @ReactiveX/rxjava-committers thoughts about this?

[stealthcode]

For invalid inputs we've typically blown up as early as possible.

[artem-zinnatullin]

I think we need to throw exception. We can highlight this in changelog and call it a bug fix because it actually is, I guess.

[stevegury]

I vote for throwing IAE.
I don't consider it's an API change, more a bug fix.

[akarnokd]

Changed the fix to throw.

[vanniktech]

Any reason for not using ExpectedException, that way the exception message can also be verified plus it can be nailed down where the Exception is thrown (the moment you call take(-1)).

[akarnokd]

Because it is irrelevant in this case, what matters is the type IAE.

[stevegury]

👍