2.x: RxJavaPlugins unwrapRunnable #5734

lukaszguz · 2017-11-21T11:12:31Z

Hi,
RxJava2 version: [2.1.6]
It is reference to #5733

akarnokd · 2017-11-21T11:20:18Z

src/main/java/io/reactivex/Scheduler.java

 import io.reactivex.disposables.Disposable;
 import io.reactivex.exceptions.Exceptions;
 import io.reactivex.functions.Function;
-import io.reactivex.internal.disposables.*;
-import io.reactivex.internal.schedulers.*;
+import io.reactivex.internal.disposables.EmptyDisposable;


Please make sure your IDE doesn't unroll star imports to reduce the diff size.

akarnokd · 2017-11-21T11:21:29Z

src/main/java/io/reactivex/internal/schedulers/SchedulerRunnableWrapper.java

+
+package io.reactivex.internal.schedulers;
+
+public interface SchedulerRunnableWrapper extends Runnable {


Please document this interface a bit and include a @since 2.1.7 - experimental tag and the @Experimental class annotation.

akarnokd · 2017-11-21T11:22:36Z

src/main/java/io/reactivex/internal/schedulers/SchedulerRunnableWrapper.java

+
+public interface SchedulerRunnableWrapper extends Runnable {
+
+    Runnable getWrappedRunnable();


Please add a short description of this method and the @Nullable annotation (some terminated wrappers do null out their wrapped Runnable instance.

akarnokd · 2017-11-21T11:23:08Z

src/main/java/io/reactivex/plugins/RxJavaPlugins.java

+     * Unwraps internal scheduler's task.
+     * @param task the internal task
+     * @return the unwrapped runnable or task
+     */


Please add @since 2.1.7 - experimental and @Experimental annotation to the method.

Alos please add @Nullable and @CheckReturnValue to the return type, @NonNull to the parameter.

akarnokd · 2017-11-21T11:24:24Z

src/test/java/io/reactivex/plugins/RxJavaPluginsTest.java

 import io.reactivex.Scheduler.Worker;
-import io.reactivex.disposables.*;
+import io.reactivex.disposables.Disposable;


Please don't unroll star imports.

codecov · 2017-11-21T11:27:13Z

Codecov Report

Merging #5734 into 2.x will increase coverage by 0.07%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##                2.x    #5734      +/-   ##
============================================
+ Coverage     96.28%   96.35%   +0.07%     
+ Complexity     5837     5833       -4     
============================================
  Files           634      634              
  Lines         41644    41650       +6     
  Branches       5768     5769       +1     
============================================
+ Hits          40096    40131      +35     
+ Misses          609      598      -11     
+ Partials        939      921      -18

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/io/reactivex/Scheduler.java	`100% <100%> (ø)`	`11 <0> (ø)`	⬇️
...activex/internal/schedulers/ExecutorScheduler.java	`98.66% <100%> (+2.04%)`	`9 <0> (ø)`	⬇️
...ctivex/internal/schedulers/AbstractDirectTask.java	`100% <100%> (ø)`	`17 <1> (+1)`	⬆️
...in/java/io/reactivex/subjects/BehaviorSubject.java	`86.97% <0%> (-5.73%)`	`57% <0%> (ø)`
.../io/reactivex/internal/functions/ObjectHelper.java	`94.73% <0%> (-5.27%)`	`20% <0%> (-1%)`
...l/operators/observable/ObservableFlatMapMaybe.java	`90.84% <0%> (-3.27%)`	`2% <0%> (ø)`
...a/io/reactivex/internal/util/QueueDrainHelper.java	`58.86% <0%> (-2.84%)`	`31% <0%> (-2%)`
...java/io/reactivex/subjects/CompletableSubject.java	`97.4% <0%> (-2.6%)`	`36% <0%> (-1%)`
...tivex/internal/schedulers/TrampolineScheduler.java	`92.3% <0%> (-2.57%)`	`6% <0%> (ø)`
...nternal/operators/parallel/ParallelSortedJoin.java	`92.75% <0%> (-2.18%)`	`2% <0%> (ø)`
... and 28 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 30905fc...3c8aefd. Read the comment docs.

akarnokd · 2017-11-21T11:28:41Z

Please also make sure the Scheduler and RxJavaPlugins reman 100% covered.

lukaszguz · 2017-11-21T14:29:14Z

Ok, I think that I've finished work with code coverage so if you will have time to review I'm open to continue discussion.

akarnokd · 2017-11-21T21:15:15Z

src/main/java/io/reactivex/Scheduler.java

@@ -368,7 +368,7 @@ public long now(@NonNull TimeUnit unit) {

            @Override
            public void run() {
-                decoratedRun.run();
+                getWrappedRunnable().run();


Please don't turn these into method calls.

Could you give me small tip? Without invoking getWrappedRunnable I don't have a 100% code coverage in Scheduler.java because I'm using the method only in RxJavaPlugins which has 100% code coverage.

When this type of task is created and returned, then invoke unwrapRunnable on it.

Do you mean something like that?

@NonNull public Disposable scheduleDirect(@NonNull Runnable run, long delay, @NonNull TimeUnit unit) { final Worker w = createWorker(); final Runnable decoratedRun = RxJavaPlugins.onSchedule(run); DisposeTask task = new DisposeTask(decoratedRun, w); w.schedule(task, delay, unit); return RxJavaPlugins.unwrapRunnable(task); }

but in that case I have incompatible types Disposable and Runnable

No, do the unwrapping in the unit test. Also for convenience, the unwrapRunnable should perhaps accept Object instead of Runnable.

akarnokd · 2017-11-21T21:16:36Z

src/test/java/io/reactivex/schedulers/SchedulerTest.java

@@ -1,48 +1,48 @@
 /**
 * Copyright (c) 2016-present, RxJava Contributors.
- *
+ * <p>


Please restore the header to the standard.

Sorry, I don't have yours code style and I have still the same problem with formating.

I corrected it.

akarnokd · 2017-11-21T21:17:17Z

src/test/java/io/reactivex/schedulers/SchedulerTest.java


 public class SchedulerTest {

    @Test
    public void defaultPeriodicTask() {
-        final int[] count = { 0 };
+        final int[] count = {0};


Please don't change unrelated tests.

Of course I will change it.

I corrected it.

akarnokd · 2017-11-22T08:27:57Z

src/main/java/io/reactivex/internal/schedulers/SchedulerRunnableWrapper.java

+ * @since 2.1.7 - experimental
+ */
+@Experimental
+public interface SchedulerRunnableWrapper extends Runnable {


I'm not certain this should extend Runnable as it may make developers think they should invoke run.

I chaged it.

akarnokd · 2017-11-22T08:29:24Z

src/test/java/io/reactivex/schedulers/SchedulerTest.java

 import io.reactivex.*;
 import io.reactivex.Scheduler.Worker;
+import io.reactivex.annotations.NonNull;


Please completely restore this file before this PR so it doesn't show up as diff at all.

I hope I've finally had the order. :)

lukaszguz · 2017-11-22T11:22:39Z

I think is ok now. I hope so :)

lukaszguz · 2017-11-22T12:31:29Z

Thank you very much for support. :)

artem-zinnatullin · 2017-11-23T00:13:39Z

Sorry I didn't get in earlier,

This API seems to be inconsistent with other introspection APIs like LambdaConsumerIntrospection where user has a publicly available interface that can be checked manually and consumed.

While this PR adds very generic method to RxJavaPlugins that can accept any Object (!) and return either null or Runnable.

Shouldn't we make interface public and document how to use it in the RxJavaPlugins. setScheduleHandler()?

Additional thought, probably no one would need it outside of RxJavaPlugins. setScheduleHandler() ie after calls like Scheduler.schedule*() since user has both original and wrapped runnable available for use.

lukaszguz · 2017-11-23T08:15:58Z

Hi,
I've described much more what I want to achieve in #5733
The main problem is that I don't have a wrapped runnable in RxJavaPlugins.scheduleHandler so when I checked if runnable of my type is in scheduleHandler I couldn't get that information because this runnable is wrapped by for example Scheduler.DisposeTask. When I was implementing I followed directions given by @akarnokd .

Do you suggest that adding better description how to use SchedulerRunnableWrapper with RxJavaPlugins will be enough?

Of course I'm open to discussion :)

lukaszguz · 2017-11-24T08:24:54Z

@akarnokd @artem-zinnatullin I made changes based on your tips.

akarnokd · 2017-11-24T08:47:43Z

src/main/java/io/reactivex/internal/schedulers/SchedulerRunnableIntrospection.java

@@ -0,0 +1,35 @@
+/**


Please move this into io.reactivex.schedulers.

akarnokd · 2017-11-24T08:48:25Z

src/test/java/io/reactivex/plugins/RxJavaPluginsTest.java

@@ -43,11 +32,22 @@
 import io.reactivex.internal.operators.observable.ObservableRange;
 import io.reactivex.internal.operators.parallel.ParallelFromPublisher;
 import io.reactivex.internal.operators.single.SingleJust;
-import io.reactivex.internal.schedulers.ImmediateThinScheduler;
+import io.reactivex.internal.schedulers.*;


Now that this doesn't have an extra method, please restore the pre-PR version.

akarnokd · 2017-11-24T09:07:40Z

src/test/java/io/reactivex/plugins/RxJavaPluginsTest.java

@@ -48,6 +37,17 @@
 import io.reactivex.observables.ConnectableObservable;
 import io.reactivex.parallel.ParallelFlowable;
 import io.reactivex.schedulers.Schedulers;
+import org.junit.*;


It still shows up as changed because the imports. Please use the version from HEAD to restore this file.

ehh it is the same mistake :)
I've fixed it.

artem-zinnatullin · 2017-11-26T00:14:16Z

src/main/java/io/reactivex/schedulers/SchedulerRunnableIntrospection.java

+import io.reactivex.plugins.RxJavaPlugins;
+
+/**
+ * Marker interface to indicate wrapped action inside internal scheduler's task.


It's not a marker interface if it declares method(s) that can be consumed :)

artem-zinnatullin · 2017-11-26T00:16:34Z

src/main/java/io/reactivex/schedulers/SchedulerRunnableIntrospection.java

+ * Inside of the {@link RxJavaPlugins#onSchedule(Runnable)}, you can unwrap runnable from internal wrappers like
+ * {@link Scheduler.DisposeTask}.
+ *
+ * @since 2.1.7 - experimental


Add "see also" with references to RxJavaPlugins.setScheduleHandler() and RxJavaPlugins. getScheduleHandler()?

What do you think about something like this?

/** * Interface to wrap an action inside internal scheduler's task. * * You can check if runnable implements this interface and unwrap original runnable. * For example inside of the {@link RxJavaPlugins#setScheduleHandler(Function)} * * @since 2.1.7 - experimental */

I corrected it.

artem-zinnatullin · 2017-11-26T00:17:37Z

src/main/java/io/reactivex/schedulers/SchedulerRunnableIntrospection.java

+/**
+ * Marker interface to indicate wrapped action inside internal scheduler's task.
+ *
+ * Inside of the {@link RxJavaPlugins#onSchedule(Runnable)}, you can unwrap runnable from internal wrappers like


Maybe change to something like:

You can check if runnable implements this interface and unwrap original runnable?

artem-zinnatullin · 2017-11-27T19:25:25Z

src/test/java/io/reactivex/schedulers/SchedulerTest.java

+        };
+        SchedulerRunnableIntrospection wrapper = (SchedulerRunnableIntrospection) scheduler.schedulePeriodicallyDirect(runnable, 100, 100, TimeUnit.MILLISECONDS);
+
+        assertEquals(runnable, wrapper.getWrappedRunnable());


assertSame?

I've changed it.

artem-zinnatullin

Pls fix comments, otherwise LGTM :)

artem-zinnatullin · 2017-11-29T23:16:40Z

src/main/java/io/reactivex/schedulers/SchedulerRunnableIntrospection.java

+ * Inside of the {@link RxJavaPlugins#onSchedule(Runnable)}, you can unwrap runnable from internal wrappers like
+ * {@link Scheduler.DisposeTask}.
+ *
+ * @since 2.1.7 - experimental


artem-zinnatullin · 2017-11-29T23:54:12Z

src/main/java/io/reactivex/schedulers/SchedulerRunnableIntrospection.java

+     *
+     * @return the wrapped action, may be null.
+     */
+    @Nullable


This actually is @NotNull now since it's not a generic Object based API, schedulers don't expect that Runnable can be null (I'm adding PR that'll enforce that)

I corrected it

artem-zinnatullin · 2017-11-29T23:54:41Z

src/test/java/io/reactivex/schedulers/SchedulerTest.java

@@ -307,4 +307,66 @@ public void customScheduleDirectDisposed() {

        assertTrue(d.isDisposed());
    }
+
+    @Test
+    public void unwrapDefaultPeriodicTask() {


Shouldn't we move these tests to AbstractSchedulerTests @akarnokd?

So I'm waiting for your decision :)

While we're waiting for @akarnokd to answer I'd recommend you to try to move this code to AbstractSchedulerTests and make sure to use getScheduler() instead of TestScheduler, that way we check that all schedulers properly support this introspection

It's fine here.

Don't we want to check that all schedulers support this API?

It can get complicated and this is an optional introspection. If @lukaszguz wants to expand it, then sure.

True, but I'd encourage @lukaszguz to try that and ask for help if needed, would be great to provide as robust API experience as possible

Maybe it's enough for now? I can always add a new PR with additional tests.

I'm fine with this as is. We can always expand the feature set later.

This is my first time when I'm doing PR to your library. Thanks to your tips, I know more or less what your requirements are and I can write better code. I think the better option would be to leave those tests in SchedulerTest because they are showed how Scheduler works but also good idea is to add tests in AbstractSchedulerTests which they will guarantee compatibility with all rx's schedulers

artem-zinnatullin · 2017-12-01T01:04:43Z

Alright, I'm not blocking this PR, just realistically these tests won't be written in future until someone face a bug (which I've now affected as an observer by making this comment lol)

…

On Thu, Nov 30, 2017, 11:54 AM lukaszguz ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/test/java/io/reactivex/schedulers/SchedulerTest.java <#5734 (comment)>: > @@ -307,4 +307,66 @@ public void customScheduleDirectDisposed() { assertTrue(d.isDisposed()); } + + @test + public void unwrapDefaultPeriodicTask() { This is my first time when I'm doing PR to your library. Thanks to your tips, I know more or less what your requirements are and I can write better code. I think the better option would be to leave those tests in SchedulerTest because they are showed how Scheduler works but also good idea is to add tests in AbstractSchedulerTests which they will guarantee compatibility with all rx's schedulers — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5734 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7B3Dy4ARqSIUhspyjU3T8eMceIEvBCks5s7wgHgaJpZM4Qlqsh> .

lukaszguz · 2017-12-01T09:39:11Z

I wrote tests to AbstractSchedulerTests :) and I found some mistakes for example in ExecutorScheduler so I added SchedulerRunnableIntrospection to AbstractDirectTask and ExecutorScheduler.DelayedRunnable. Now I think it has already work for all rx's schedulers.
I almost forgot I have left unwrapped test in SchedulerTest I think they show a lot.

akarnokd · 2017-12-01T10:53:39Z

src/main/java/io/reactivex/internal/schedulers/ExecutorScheduler.java

+
+        @Override
+        public Runnable getWrappedRunnable() {
+            return get();


dispose() will swap in a null but the method is defined as @NotNull. So either the interface should allow nulls or this should return Functions.EMPTY_RUNNABLE:

Runnable r = get(); return r != null ? r : Functions.EMPTY_RUNNABLE;

Ehhh I did not notice it. I will check what will happen when dispose return Functions.EMPTY_RUNNABLE

All tests were passed with this change. I will leave with:

Runnable r = get(); return r != null ? r : Functions.EMPTY_RUNNABLE;

artem-zinnatullin · 2017-12-03T22:17:09Z

src/main/java/io/reactivex/internal/schedulers/ExecutorScheduler.java

+        @Override
+        public Runnable getWrappedRunnable() {
+            Runnable r = get();
+            return r != null ? r : Functions.EMPTY_RUNNABLE;


Alternative solution could be to throw IllegalStateException I guess

This is not a failure state but a terminal state of the wrapper. Penalizing it would be too harsh.

For this particular introspection api it seems like an invalid state, but I'm ~fine with current solution.

The problem of returning empty runnable is that since it's the same object every time — it can mess up introspection that is based on object identity/equality

artem-zinnatullin · 2017-12-03T22:18:39Z

Thanks for addressing feedback!

2.x: RxJavaPlugins unwrapRunnable

d0589d4

akarnokd requested changes Nov 21, 2017

View reviewed changes

akarnokd added 2.x Enhancement labels Nov 21, 2017

akarnokd added this to the 2.2 milestone Nov 21, 2017

lguz added 6 commits November 21, 2017 12:57

2.x: RxJavaPlugins unwrapRunnable-javadoc

3be3fa0

2.x: RxJavaPlugins unwrapRunnable-Enhancement and test coverage

f8ef988

2.x: RxJavaPlugins unwrapRunnable-Enhancement and test coverage

e862c1f

2.x: RxJavaPlugins unwrapRunnable-Enhancement and test coverage

07cbd46

2.x: RxJavaPlugins unwrapRunnable-Enhancement and test coverage

31ff1e9

2.x: RxJavaPlugins unwrapRunnable-Enhancement and test coverage

d5c2166

akarnokd requested changes Nov 21, 2017

View reviewed changes

lguz added 4 commits November 22, 2017 08:42

2.x: RxJavaPlugins unwrapRunnable

a75a7f1

2.x: RxJavaPlugins unwrapRunnable

b47fcd8

2.x: RxJavaPlugins unwrapRunnable

c0d6c78

2.x: RxJavaPlugins unwrapRunnable

f63a799

akarnokd reviewed Nov 22, 2017

View reviewed changes

2.x: RxJavaPlugins unwrapRunnable

1db27f8

akarnokd approved these changes Nov 22, 2017

View reviewed changes

lukaszguz closed this Nov 22, 2017

lukaszguz reopened this Nov 22, 2017

lukaszguz mentioned this pull request Nov 22, 2017

RxJava2 Autoconfiguration spring-cloud/spring-cloud-sleuth#775

Closed

2.x: RxJavaPlugins unwrapRunnable

9703201

akarnokd reviewed Nov 24, 2017

View reviewed changes

2.x: RxJavaPlugins unwrapRunnable

a5a7ce0

akarnokd reviewed Nov 24, 2017

View reviewed changes

2.x: RxJavaPlugins unwrapRunnable

ebe2ca8

akarnokd approved these changes Nov 24, 2017

View reviewed changes

artem-zinnatullin reviewed Nov 27, 2017

View reviewed changes

2.x: RxJavaPlugins unwrapRunnable

b2f0c86

artem-zinnatullin reviewed Nov 29, 2017

View reviewed changes

This was referenced Nov 30, 2017

2.x: Fix TrampolineScheduler not calling RxJavaPlugins.onSchedule(), add tests for all schedulers. #5747

Merged

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

Merged

lguz added 2 commits November 30, 2017 08:53

2.x: RxJavaPlugins unwrapRunnable

bf72b36

2.x: RxJavaPlugins unwrapRunnable

171ac80

lguz added 3 commits December 1, 2017 09:47

Merge remote-tracking branch 'upstream/2.x' into 2.x

19eba48

2.x: RxJavaPlugins unwrapRunnable

2a8e6f0

2.x: RxJavaPlugins unwrapRunnable

f1af3f7

2.x: RxJavaPlugins unwrapRunnable

a1c5ad1

akarnokd reviewed Dec 1, 2017

View reviewed changes

lguz added 2 commits December 1, 2017 12:44

2.x: RxJavaPlugins unwrapRunnable

ac5c3f5

2.x: RxJavaPlugins unwrapRunnable

3c8aefd

artem-zinnatullin approved these changes Dec 3, 2017

View reviewed changes

akarnokd merged commit 4d2e821 into ReactiveX:2.x Dec 4, 2017

akarnokd mentioned this pull request Dec 4, 2017

How to check type of Runnable inside the RxJavaPlugins onScheduleHandler #5733

Closed

akarnokd mentioned this pull request Dec 18, 2017

2.1.8 release preparations #5769

Closed

jared-bot mentioned this pull request Dec 27, 2017

Dependency update: rxjava xmartlabs/bigbang-template#45

Merged


		package io.reactivex.internal.schedulers;

		public interface SchedulerRunnableWrapper extends Runnable {


		public interface SchedulerRunnableWrapper extends Runnable {

		Runnable getWrappedRunnable();

2.x: RxJavaPlugins unwrapRunnable #5734

2.x: RxJavaPlugins unwrapRunnable #5734

Conversation

lukaszguz commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 21, 2017 • edited Loading

Codecov Report

akarnokd commented Nov 21, 2017

lukaszguz commented Nov 21, 2017

Choose a reason for hiding this comment

lukaszguz Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaszguz Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaszguz commented Nov 22, 2017

lukaszguz commented Nov 22, 2017

artem-zinnatullin commented Nov 23, 2017

lukaszguz commented Nov 23, 2017

lukaszguz commented Nov 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaszguz Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-zinnatullin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-zinnatullin commented Dec 1, 2017 via email

lukaszguz commented Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 21, 2017 •

edited

Loading

lukaszguz Nov 22, 2017 •

edited

Loading

lukaszguz Nov 22, 2017 •

edited

Loading

lukaszguz Nov 24, 2017 •

edited

Loading

lukaszguz commented Dec 1, 2017 •

edited

Loading