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.

Sign up for GitHub

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

2.x: Feature/#4876 more null checks #5055

Merged
merged 12 commits into from
Feb 3, 2017
+46 −13
Merged

2.x: Feature/#4876 more null checks #5055

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3564b1c
add NotNull annotations
jschneider Feb 3, 2017
4a03752
avoid false positive
jschneider Feb 3, 2017
23185c3
add annotations and assert statement to help static code analysis
jschneider Feb 3, 2017
91820c0
remove redundant check
jschneider Feb 3, 2017
a05a1ad
mark parameter as nullable
jschneider Feb 3, 2017
96c1680
add test to reproduce npe
jschneider Feb 3, 2017
0d0c0ea
add null check for avoid npe
jschneider Feb 3, 2017
caf7309
parameter time unit marked as @NonNull
jschneider Feb 3, 2017
79e9855
add annotations and assert to help static code analysis
jschneider Feb 3, 2017
e3e824a
remove assert statements
jschneider Feb 3, 2017
6acef0d
add missing annotation
jschneider Feb 3, 2017
339b6b8
add comment for test case
jschneider Feb 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/main/java/io/reactivex/internal/operators/flowable/FlowableCombineLatest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import java.util.Iterator;
import java.util.concurrent.atomic.*;

import io.reactivex.annotations.NonNull;
import io.reactivex.annotations.Nullable;
import org.reactivestreams.*;

Expand All @@ -37,8 +38,10 @@
public final class FlowableCombineLatest<T, R>
extends Flowable<R> {

@Nullable
final Publisher<? extends T>[] array;

@Nullable
final Iterable<? extends Publisher<? extends T>> iterable;

final Function<? super Object[], ? extends R> combiner;
Expand All @@ -47,8 +50,8 @@ public final class FlowableCombineLatest<T, R>

final boolean delayErrors;

public FlowableCombineLatest(Publisher<? extends T>[] array,
Function<? super Object[], ? extends R> combiner,
public FlowableCombineLatest(@NonNull Publisher<? extends T>[] array,
@NonNull Function<? super Object[], ? extends R> combiner,
int bufferSize, boolean delayErrors) {
this.array = array;
this.iterable = null;
Expand All @@ -57,8 +60,8 @@ public FlowableCombineLatest(Publisher<? extends T>[] array,
this.delayErrors = delayErrors;
}

public FlowableCombineLatest(Iterable<? extends Publisher<? extends T>> iterable,
Function<? super Object[], ? extends R> combiner,
public FlowableCombineLatest(@NonNull Iterable<? extends Publisher<? extends T>> iterable,
@NonNull Function<? super Object[], ? extends R> combiner,
int bufferSize, boolean delayErrors) {
this.array = null;
this.iterable = iterable;
Expand All @@ -73,6 +76,7 @@ public void subscribeActual(Subscriber<? super R> s) {
Publisher<? extends T>[] a = array;
int n;
if (a == null) {
assert iterable != null; //either array or iterable are initialized with non null values
Copy link
Member

Choose a reason for hiding this comment

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

Asserts are off by default but they increase the bytecode size; besides we don't use assert it anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. removed it.
Do you have a standard way to suppress false positives?

n = 0;
a = new Publisher[8];

Expand Down
1 change: 1 addition & 0 deletions src/main/java/io/reactivex/internal/operators/flowable/FlowableWindow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ public void onNext(T t) {

if (i == size) {
window = null;
assert w != null;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove these asserts.

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

w.onComplete();
}

Expand Down
10 changes: 7 additions & 3 deletions src/main/java/io/reactivex/internal/operators/flowable/FlowableWithLatestFromMany.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
import java.util.Arrays;
import java.util.concurrent.atomic.*;

import io.reactivex.annotations.NonNull;
import io.reactivex.annotations.Nullable;
import org.reactivestreams.*;

import io.reactivex.disposables.Disposable;
Expand All @@ -33,21 +35,22 @@
* @param <R> the output type
*/
public final class FlowableWithLatestFromMany<T, R> extends AbstractFlowableWithUpstream<T, R> {

@Nullable
final Publisher<?>[] otherArray;

@Nullable
final Iterable<? extends Publisher<?>> otherIterable;

final Function<? super Object[], R> combiner;

public FlowableWithLatestFromMany(Publisher<T> source, Publisher<?>[] otherArray, Function<? super Object[], R> combiner) {
public FlowableWithLatestFromMany(@NonNull Publisher<T> source, @NonNull Publisher<?>[] otherArray, Function<? super Object[], R> combiner) {
super(source);
this.otherArray = otherArray;
this.otherIterable = null;
this.combiner = combiner;
}

public FlowableWithLatestFromMany(Publisher<T> source, Iterable<? extends Publisher<?>> otherIterable, Function<? super Object[], R> combiner) {
public FlowableWithLatestFromMany(@NonNull Publisher<T> source, @NonNull Iterable<? extends Publisher<?>> otherIterable, Function<? super Object[], R> combiner) {
Copy link
Member

Choose a reason for hiding this comment

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

combiner needs annotation

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

super(source);
this.otherArray = null;
this.otherIterable = otherIterable;
Expand All @@ -59,6 +62,7 @@ protected void subscribeActual(Subscriber<? super R> s) {
Publisher<?>[] others = otherArray;
int n = 0;
if (others == null) {
assert otherIterable != null;
others = new Publisher[8];

try {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/reactivex/internal/operators/maybe/MaybeFlatMapIterableObservable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ public void onSuccess(T value) {

this.it = iter;

if (outputFused && iter != null) {
if (outputFused) {
a.onNext(null);
a.onComplete();
return;
Expand Down
10 changes: 8 additions & 2 deletions src/main/java/io/reactivex/internal/operators/observable/ObservableWithLatestFromMany.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
import java.util.concurrent.atomic.*;

import io.reactivex.*;
import io.reactivex.annotations.NonNull;
import io.reactivex.annotations.Nullable;
import io.reactivex.disposables.Disposable;
import io.reactivex.exceptions.Exceptions;
import io.reactivex.functions.Function;
Expand All @@ -33,20 +35,23 @@
*/
public final class ObservableWithLatestFromMany<T, R> extends AbstractObservableWithUpstream<T, R> {

@Nullable
final ObservableSource<?>[] otherArray;

@Nullable
final Iterable<? extends ObservableSource<?>> otherIterable;

@NonNull
final Function<? super Object[], R> combiner;

public ObservableWithLatestFromMany(ObservableSource<T> source, ObservableSource<?>[] otherArray, Function<? super Object[], R> combiner) {
public ObservableWithLatestFromMany(@NonNull ObservableSource<T> source, @NonNull ObservableSource<?>[] otherArray, @NonNull Function<? super Object[], R> combiner) {
super(source);
this.otherArray = otherArray;
this.otherIterable = null;
this.combiner = combiner;
}

public ObservableWithLatestFromMany(ObservableSource<T> source, Iterable<? extends ObservableSource<?>> otherIterable, Function<? super Object[], R> combiner) {
public ObservableWithLatestFromMany(@NonNull ObservableSource<T> source, @NonNull Iterable<? extends ObservableSource<?>> otherIterable, @NonNull Function<? super Object[], R> combiner) {
super(source);
this.otherArray = null;
this.otherIterable = otherIterable;
Expand All @@ -58,6 +63,7 @@ protected void subscribeActual(Observer<? super R> s) {
ObservableSource<?>[] others = otherArray;
int n = 0;
if (others == null) {
assert otherIterable!=null;
others = new ObservableSource[8];

try {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/reactivex/internal/schedulers/ComputationScheduler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ public Disposable schedule(@NonNull Runnable action) {
return EmptyDisposable.INSTANCE;
}

return poolWorker.scheduleActual(action, 0, null, serial);
return poolWorker.scheduleActual(action, 0, TimeUnit.MILLISECONDS, serial);
}
@NonNull
@Override
Expand Down
8 changes: 6 additions & 2 deletions src/main/java/io/reactivex/internal/schedulers/NewThreadWorker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import io.reactivex.Scheduler;
import io.reactivex.annotations.NonNull;
import io.reactivex.annotations.Nullable;
import io.reactivex.disposables.*;
import io.reactivex.internal.disposables.*;
import io.reactivex.plugins.RxJavaPlugins;
Expand Down Expand Up @@ -106,7 +107,8 @@ public Disposable schedulePeriodicallyDirect(final Runnable run, long initialDel
* @param parent the optional tracker parent to add the created ScheduledRunnable instance to before it gets scheduled
* @return the ScheduledRunnable instance
*/
public ScheduledRunnable scheduleActual(final Runnable run, long delayTime, TimeUnit unit, DisposableContainer parent) {
@NonNull
public ScheduledRunnable scheduleActual(final Runnable run, long delayTime, @NonNull TimeUnit unit, @Nullable DisposableContainer parent) {
Runnable decoratedRun = RxJavaPlugins.onSchedule(run);

ScheduledRunnable sr = new ScheduledRunnable(decoratedRun, parent);
Expand All @@ -126,7 +128,9 @@ public ScheduledRunnable scheduleActual(final Runnable run, long delayTime, Time
}
sr.setFuture(f);
} catch (RejectedExecutionException ex) {
parent.remove(sr);
if (parent != null) {
parent.remove(sr);
}
RxJavaPlugins.onError(ex);
}

Expand Down
14 changes: 14 additions & 0 deletions src/test/java/io/reactivex/schedulers/NewThreadSchedulerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,18 @@ public void run() {

assertEquals(0, calls[0]);
}

@Test
public void npe() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

This could be named a bit more informatively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep. added some comments

Scheduler s = getScheduler();
NewThreadWorker w = (NewThreadWorker) s.createWorker();
w.dispose();

w.scheduleActual(new Runnable() {
@Override
public void run() {
}
}, 0, TimeUnit.MILLISECONDS, null);

}
}