2.x: Subject NPE fixes, add UnicastProcessor TCK #5760
Conversation
Codecov Report
@@ Coverage Diff @@
## 2.x #5760 +/- ##
===========================================
- Coverage 96.24% 96.2% -0.05%
+ Complexity 5838 5808 -30
===========================================
Files 634 634
Lines 41653 41607 -46
Branches 5769 5770 +1
===========================================
- Hits 40090 40028 -62
- Misses 624 636 +12
- Partials 939 943 +4
Continue to review full report at Codecov.
|
| @@ -175,10 +175,8 @@ public void onSubscribe(Subscription s) { | |||
|
|
|||
| @Override | |||
| public void onNext(T t) { | |||
| if (t == null) { | |||
| onError(new NullPointerException("onNext called with null. Null values are generally not allowed in 2.x operators and sources.")); | |||
There was a problem hiding this comment.
😰😰😰 well this is a little bit breaking change, but since it was always part of RS spec I guess it falls into patch category for RxJava
There was a problem hiding this comment.
You were not supposed to call onNext with null anyway and there is no reason to take this extra long path to get the end subscriber a NullPointerException via onError.
There was a problem hiding this comment.
Yep I understand :)
Just noted that it's a little bit breaking behavior for existing users, but since RS doesn't allow nulls anyway we can make this change as "patch" rather than wait for 3.x
|
I like RS spec-related fixes 👍 But not sure about motivation for having |
|
Other than such the behavior is needed to pass the TCK, none that I can think of. The original idea behind I can move the operator into the test section and not extend the |
That sounds much better to me tbh, I just don't want us to decline PRs with new operators from users and suggest them contribute to rxjava-extras/etc and at the same time add new ones that no user really asked for Nothing personal :) |
|
Okay, I'll have a copy of the processor version in test and move both into RxJava2Extensions. |
|
Thanks, David! Appreciate that |
akarnokd
changed the title
|
Done. |
| .assertNoValues() | ||
| .assertError(NullPointerException.class) | ||
| .assertErrorMessage("onError called with null. Null values are generally not allowed in 2.x operators and sources."); | ||
| p.test().assertEmpty().cancel();; |
There was a problem hiding this comment.
nit ;; (and in other places in this PR)
| * @param <T> the upstream and downstream value type | ||
| * @since 2.1.8 - experimental | ||
| */ | ||
| @Experimental |
|
// Now started watching rxjava-extras, feel free to ping me if you need review on a PR there |
|
I put my excess things into RxJava2Extensions btw. rxjava2-extras is @davidmoten 's repository. |
|
yep, I've mixed up naming but subscribed to yours. Somehow I've never needed to use neither of extensions/extras for all these years 🤔 |
This PR adds the
Subject.refCount()andFlowableProcessor.refCount()that capture the upstream'sDisposable/Subscriptionand disposes/cancels them if the number ofObservers/Subscribers decreases to zero. The Reactive-Streams TCK and thus other implementations may expect such behavior from aProcessorimplementation and this wrapper is required to pass the TCK tests.While implementing the TCK tests, it turned out the
null-handling of theSubjects andFlowableProcessors were not following the Reactive-Streams specification. They have to throw aNullPointerExceptionimmediately and not turn them into NPEs for the downstream. These classes and the tests have been fixed as well. Their error messages have been uniformed too.