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

Fail early if a null subscription is added to a CompositeSubscription. #2447

Merged
merged 4 commits into from
Jan 21, 2015
Merged

Conversation

jnlopar
Copy link
Contributor

@jnlopar jnlopar commented Jan 12, 2015

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.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation should be four spaces here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks!

@@ -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.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use NullPointerException. Other null assertions in RxJava throws NPE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done! :)

@akarnokd
Copy link
Member

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?

@jnlopar
Copy link
Contributor Author

jnlopar commented Jan 20, 2015

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?

@akarnokd
Copy link
Member

Unsubscribed is a terminal state and can't be revived.

@akarnokd
Copy link
Member

Thanks. Merging.

akarnokd added a commit that referenced this pull request Jan 21, 2015
Fail early if a null subscription is added to a CompositeSubscription.
@akarnokd akarnokd merged commit 2af4f74 into ReactiveX:1.x Jan 21, 2015
@benjchristensen benjchristensen mentioned this pull request Feb 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants