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

Jump to bottom

2.x: Check runnable == null in *Scheduler.schedule*(). #5748

Merged
merged 5 commits into from
Dec 7, 2017
Merged

2.x: Check runnable == null in *Scheduler.schedule*(). #5748

merged 5 commits into from
Dec 7, 2017

Conversation

artem-zinnatullin
Copy link
Contributor

@artem-zinnatullin artem-zinnatullin force-pushed the az/2.x/schedule-nullable-runnable branch from 933fb62 to 9affb70 Compare November 30, 2017 02:07
artem-zinnatullin
artem-zinnatullin commented Nov 30, 2017
View reviewed changes
src/test/java/io/reactivex/plugins/RxJavaPluginsTest.java
@@ -1381,10 +1379,7 @@ public void subscribeActual(MaybeObserver t) {
assertSame(myb, RxJavaPlugins.onAssembly(myb));


assertNull(RxJavaPlugins.onSchedule(null));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what was the reason to check null twice here

@codecov
Copy link

codecov bot commented Nov 30, 2017 •
edited
Loading

Codecov Report

Merging #5748 into 2.x will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##                2.x   #5748      +/-   ##
===========================================
- Coverage     96.31%   96.3%   -0.02%     
+ Complexity     5840    5839       -1     
===========================================
  Files           634     634              
  Lines         41650   41651       +1     
  Branches       5769    5769              
===========================================
- Hits          40115   40111       -4     
+ Misses          612     610       -2     
- Partials        923     930       +7
Impacted Files Coverage Δ Complexity Δ
.../main/java/io/reactivex/plugins/RxJavaPlugins.java 100% <100%> (ø) 146 <2> (ø) ⬇️
.../operators/flowable/FlowableBlockingSubscribe.java 91.89% <0%> (-5.41%) 9% <0%> (-1%)
...ternal/operators/observable/ObservablePublish.java 92.98% <0%> (-3.51%) 11% <0%> (ø)
...rnal/operators/flowable/FlowableTakeLastTimed.java 96.29% <0%> (-2.78%) 2% <0%> (ø)
...rnal/operators/flowable/FlowableFlatMapSingle.java 94.56% <0%> (-2.18%) 2% <0%> (ø)
...ernal/operators/flowable/FlowableFromIterable.java 95.18% <0%> (-2.14%) 5% <0%> (ø)
.../internal/disposables/ListCompositeDisposable.java 98% <0%> (-2%) 34% <0%> (-1%)
...ex/internal/subscribers/InnerQueuedSubscriber.java 96.07% <0%> (-1.97%) 18% <0%> (-1%)
.../io/reactivex/disposables/CompositeDisposable.java 97.24% <0%> (-1.84%) 39% <0%> (-1%)
...java/io/reactivex/processors/PublishProcessor.java 98.27% <0%> (-1.73%) 45% <0%> (-1%)
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53d5a23...c91b338. Read the comment docs.

artem-zinnatullin
artem-zinnatullin commented Nov 30, 2017
View reviewed changes
src/main/java/io/reactivex/plugins/RxJavaPlugins.java Outdated
@@ -446,6 +446,8 @@ public static Scheduler onNewThreadScheduler(@NonNull Scheduler defaultScheduler
*/
@NonNull
public static Runnable onSchedule(@NonNull Runnable run) {
ObjectHelper.requireNonNull(run, "runnable is null");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to add explanation to PR why I've added check here and not in Scheduler.schedule*() methods:

Reason behind that is abstract Scheduler has 3 overloads, one of them reuses another one + any concrete scheduler can override any of those methods and call other overloads, so as a result I had checks duplicated in some cases.

This RxJavaPlugins call is done by all schedulers before actual schedule/run of the original Runnable so it seems to be a good common point that only executed once and guarantees now that Runnable is not null for both user defined RxJavaPlugins.onSchedule() function and all RxJava schedulers.

akarnokd
akarnokd previously requested changes Dec 4, 2017
View reviewed changes
src/main/java/io/reactivex/internal/schedulers/TrampolineScheduler.java Outdated
@@ -49,13 +49,17 @@ public Worker createWorker() {
@NonNull
@Override
public Disposable scheduleDirect(@NonNull Runnable run) {
// TODO remove if https://github.com/ReactiveX/RxJava/pull/5747 gets merged.
ObjectHelper.requireNonNull(run, "runnable is 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 "run is null", but otherwise the run.run() below would have crashed anyway.

@akarnokd
Copy link
Member

akarnokd commented Dec 6, 2017

@artem-zinnatullin Could you rebase this PR and reapply your changes?

@akarnokd akarnokd dismissed their stale review December 6, 2017 12:55

outdated

@artem-zinnatullin
Copy link
Contributor Author

Done, sorry for delay.

@akarnokd akarnokd merged commit 564c3dc into ReactiveX:2.x Dec 7, 2017
@akarnokd akarnokd added this to the 2.2 milestone Dec 7, 2017
@akarnokd akarnokd mentioned this pull request Dec 18, 2017
Closed
@jared-bot jared-bot mentioned this pull request Dec 27, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akarnokd akarnokd akarnokd approved these changes

@vanniktech vanniktech vanniktech approved these changes

Assignees
No one assigned
Labels
2.x Cleanup
Projects
None yet
Milestone
2.2
Development

Successfully merging this pull request may close these issues.
3 participants
@artem-zinnatullin @akarnokd @vanniktech