From 5154351f2c74645cd336da58af06ac81e55f0607 Mon Sep 17 00:00:00 2001 From: Jason Neufeld Date: Mon, 12 Jan 2015 14:33:48 -0800 Subject: [PATCH 1/4] 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. --- src/main/java/rx/subscriptions/CompositeSubscription.java | 3 +++ .../java/rx/subscriptions/CompositeSubscriptionTest.java | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/src/main/java/rx/subscriptions/CompositeSubscription.java b/src/main/java/rx/subscriptions/CompositeSubscription.java index fea7b70910..02f54da17c 100644 --- a/src/main/java/rx/subscriptions/CompositeSubscription.java +++ b/src/main/java/rx/subscriptions/CompositeSubscription.java @@ -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."); + } Subscription unsubscribe = null; synchronized (this) { if (unsubscribed) { diff --git a/src/test/java/rx/subscriptions/CompositeSubscriptionTest.java b/src/test/java/rx/subscriptions/CompositeSubscriptionTest.java index 4cff2a3e6a..c69589cb31 100644 --- a/src/test/java/rx/subscriptions/CompositeSubscriptionTest.java +++ b/src/test/java/rx/subscriptions/CompositeSubscriptionTest.java @@ -337,4 +337,11 @@ public void testTryRemoveIfNotIn() { csub.remove(csub1); // try removing agian } + + @Test(expected = IllegalArgumentException.class) + public void testAddingNullSubscriptionIllegal() { + CompositeSubscription csub = new CompositeSubscription(); + csub.add(null); + } + } From ed63daa9520b06f574eb387a43c2c2620bc70134 Mon Sep 17 00:00:00 2001 From: Jason Neufeld Date: Mon, 12 Jan 2015 14:58:49 -0800 Subject: [PATCH 2/4] Fixes indentation. --- src/test/java/rx/subscriptions/CompositeSubscriptionTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/rx/subscriptions/CompositeSubscriptionTest.java b/src/test/java/rx/subscriptions/CompositeSubscriptionTest.java index c69589cb31..ad95939b22 100644 --- a/src/test/java/rx/subscriptions/CompositeSubscriptionTest.java +++ b/src/test/java/rx/subscriptions/CompositeSubscriptionTest.java @@ -340,8 +340,8 @@ public void testTryRemoveIfNotIn() { @Test(expected = IllegalArgumentException.class) public void testAddingNullSubscriptionIllegal() { - CompositeSubscription csub = new CompositeSubscription(); - csub.add(null); + CompositeSubscription csub = new CompositeSubscription(); + csub.add(null); } } From 224cafb256ad1f0c34741cd1bb094bc4d9584142 Mon Sep 17 00:00:00 2001 From: Jason Neufeld Date: Mon, 12 Jan 2015 18:24:41 -0800 Subject: [PATCH 3/4] IllegalArgumentException > NPE --- src/main/java/rx/subscriptions/CompositeSubscription.java | 2 +- src/test/java/rx/subscriptions/CompositeSubscriptionTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/rx/subscriptions/CompositeSubscription.java b/src/main/java/rx/subscriptions/CompositeSubscription.java index 02f54da17c..d69a413554 100644 --- a/src/main/java/rx/subscriptions/CompositeSubscription.java +++ b/src/main/java/rx/subscriptions/CompositeSubscription.java @@ -56,7 +56,7 @@ public synchronized boolean isUnsubscribed() { */ public void add(final Subscription s) { if (s == null) { - throw new IllegalArgumentException("Added Subscription cannot be null."); + throw new NullPointerException("Added Subscription cannot be null."); } Subscription unsubscribe = null; synchronized (this) { diff --git a/src/test/java/rx/subscriptions/CompositeSubscriptionTest.java b/src/test/java/rx/subscriptions/CompositeSubscriptionTest.java index ad95939b22..18ac45f198 100644 --- a/src/test/java/rx/subscriptions/CompositeSubscriptionTest.java +++ b/src/test/java/rx/subscriptions/CompositeSubscriptionTest.java @@ -338,7 +338,7 @@ public void testTryRemoveIfNotIn() { csub.remove(csub1); // try removing agian } - @Test(expected = IllegalArgumentException.class) + @Test(expected = NullPointerException.class) public void testAddingNullSubscriptionIllegal() { CompositeSubscription csub = new CompositeSubscription(); csub.add(null); From 14fcc22ec431e87b88f99a7d5498c321127dd48d Mon Sep 17 00:00:00 2001 From: Jason Neufeld Date: Tue, 20 Jan 2015 13:13:55 -0800 Subject: [PATCH 4/4] Use unsubscribed check instead of a null check. --- .../java/rx/subscriptions/CompositeSubscription.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/rx/subscriptions/CompositeSubscription.java b/src/main/java/rx/subscriptions/CompositeSubscription.java index d69a413554..777b18f04d 100644 --- a/src/main/java/rx/subscriptions/CompositeSubscription.java +++ b/src/main/java/rx/subscriptions/CompositeSubscription.java @@ -1,12 +1,12 @@ /** * Copyright 2014 Netflix, Inc. - * + * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -55,8 +55,8 @@ public synchronized boolean isUnsubscribed() { * the {@link Subscription} to add */ public void add(final Subscription s) { - if (s == null) { - throw new NullPointerException("Added Subscription cannot be null."); + if (s.isUnsubscribed()) { + return; } Subscription unsubscribe = null; synchronized (this) {