Fail early if a null subscription is added to a CompositeSubscription. #2447
Conversation
Otherwise, it'll just fail late when unsubscribing, which is much harder to trace.
| @Test(expected = IllegalArgumentException.class) | ||
| public void testAddingNullSubscriptionIllegal() { | ||
| CompositeSubscription csub = new CompositeSubscription(); | ||
| csub.add(null); |
There was a problem hiding this comment.
Indentation should be four spaces here.
| @@ -55,6 +55,9 @@ public synchronized boolean isUnsubscribed() { | |||
| * the {@link Subscription} to add | |||
| */ | |||
| public void add(final Subscription s) { | |||
| if (s == null) { | |||
| throw new IllegalArgumentException("Added Subscription cannot be null."); | |||
There was a problem hiding this comment.
It's better to use NullPointerException. Other null assertions in RxJava throws NPE.
|
I suggest an alternative to get another effect: adding an already unsubscribed Subscription is essentially no-op and just blocks others adding regular Subscriptions concurrently. How about do an isUnsubscribed check in add: public void add(Subscription s) {
if (s.isUnsubscribed()) {
return;
}
}Certainly, this will get you NPE. I read somewhere that JIT may completely remove null-checks and simply relying on page faults of address 0. The drawback is that the NPE indicates an error in RxJava now instead of the contract violation. Thoughts? |
|
Done. Good thought. One concern though, is it possible for an unsubscribed subscription to become subscribed again? I can't think of a case where that happens, but if it could, would this still be desired behavior? |
|
Unsubscribed is a terminal state and can't be revived. |
|
Thanks. Merging. |
Fail early if a null subscription is added to a CompositeSubscription.
Otherwise, it'll just fail late when unsubscribing, which is much harder to trace.
I discovered this while writing a unit test, when I didn't properly mock out a method to return a valid Observable. Seems like it would be more likely to happen in test code than production, but still could be a stumbling block that's hard to track down if there's a bug in app logic adding a null subscription.