1.x: new hook management proposal

[akarnokd]

This PR adds a new, in-between, hook manager class, RxJavaHooks that allows runtime hooking of Observable, Single, Completable and Schedulers components and is aimed to be more versatile than the RxJavaPlugins.

Since RxJavaPlugins is public API, it can't be just removed, therefore, RxJavaHooks by default delegates to it but that delegation can be completely disabled via clear().

Usage

Call the setOn methods with an appropriate function to manipulate the object being hooked:

RxJavaHooks.setOnComputationScheduler(s -> Schedulers.immediate());

RxJavaHooks.setObservableCreate(o -> { System.out.println("Creating: " + o); return o; });

You can also get the current hooks via the getOn methods, allowing chaining multiple hooks if necessary. Changing and retrieving the hooks are thread safe, although it is recommended you change them in quiet times.

Operator writers should usually not call the onXXX methods on RxJavaHooks except RxJavaHooks.onError(); It is useful when they have to signal a Throwable that can't be delivered to a Subscriber.

Calling reset() will restore the original delegation hooks.

This PR also features a way of tracking the assembly locations, that is where Observable.create() was invoked internally or externally, via enableAssemblyTracking. It changes the creation hooks of all 3 base types by adding an OnSubscribeOnAssemblyX wrapper. This operator will replace the Throwable flowing through onError and wraps it via the new AssemblyStackTraceException.

Both the wrapper and the exception contain a string representation of the captured stacktrace, this helps "printing" out that information in a debugging session by simply viewing the field contents.

This tracking can be enabled at any time and affects sequences created afterwards. To stop the tracking, use resetAssemblyTracking to restore the default delegate callbacks to RxJavaPlugins or clearAssemblyTracking to restore the empty hooks (only affecting the onCreate hooks).

For frameworks, the RxJavaHooks can be locked down, preventing changing the hooks.

If you are using RxJavaPlugins existing features, you don't have to do anything; tests will work as before.

Performance impact

A default RxJavaHooks incurs a 2-4 level indirection in method calls. onNext calls are not affected.

A clear RxJavaHooks incurs a volatile read (very cheap) followed by a branch (predictable). onNext calls are not affected.

A tracking-enabled RxJavaHooks incurs an estimated 1000-3000 cycles overhead for each source creation and operator application. The AssemblyStackTraceException itself doesn't fill in its own stacktrace but takes only the captured stacktrace string and has mostly the cost of an object allocation. Due to the in-between nature of tracking, onXXX calls get through another indirection at each operator. For example, range(1, 5).map(v -> v).filter(v -> true).subscribe() will have 3 extra layers (one for each .). I believe these are acceptable overheads because the tracking feature is for tracking down crashes and not performance problems.

Discussion

Not all hook methods have been replicated completely on RxJavaHooks. For one, I wanted a minimal working prototype that passes existing tests. Second, those that are left out are not even tested (onSubscribeError, onLift). If the concept of this PR is accepted, those can be added along with their unit tests. Also further Javadoc will be added in the same case.

Names and structure are not set in stone.

[artem-zinnatullin]

👍

[artem-zinnatullin]

👍 // More tests would be great, especially for removing parts of stacktrace, but otherwise LGTM.

[akarnokd]

// More tests would be great, especially for removing parts of stacktrace, but otherwise LGTM.

Do you have tips how to fake stacktrace for Thread.currentThread().getStacktrace() ? Otherwise, there might be other common frameworks that could be filtered, such as Android stuff.

[artem-zinnatullin]

Accept StackTraceElement[] in the function and pass fake StackTraceElement[] in tests?

[akarnokd]

If this PR gets a go ahead, I'll do that.

[artem-zinnatullin]

@hzsweers PTAL

[ZacSweers]

Will look when I can, but probably won't be until next week due to work projects.

[akarnokd]

Also it is possible to merge RxJavaHooks into RxJavaPlugins. It would avoid some confusion when switching to 2.x plugins API.

[artem-zinnatullin]

Also it is possible to merge RxJavaHooks into RxJavaPlugins. It would avoid some confusion when switching to 2.x plugins API.

Don't you afraid that it'll make API of RxJavaPlugins too confusing?

[ZacSweers]

How would it make it more confusing?

[artem-zinnatullin]

Either we'll need to break current API or add very similar methods to
existing which will make it confusing.

On Wed, 22 Jun 2016, 12:22 Zac Sweers, [email protected] wrote:

How would it make it more confusing?

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#4007 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AA7B3LHg08TP9iNMUuP_9AD1Zhl2Y2e7ks5qOP67gaJpZM4I1YE9
.

[akarnokd]

It's an odd class either way. Plus, I've seen mentioned that few tests actually extend RxJavaPlugins. I'm leaning towards having RxJavaHooks separate and clearly marked @Experimental.

[akarnokd]

Updated with more Javadoc.

[codecov-io]

Current coverage is 80.26%

No coverage report found for 1.x at 08a2130.

Powered by Codecov. Last updated by 08a2130...99b256c

[akarnokd]

I'd like to merge this so we can make progress on the coverage/PMD front (which will indicate what tests are needed for this). Any objections?

[ZacSweers]

I'll take a good look at it today if you don't mind waiting a few hours

[akarnokd]

Sure, take your time.

[ZacSweers]

Does this mean it's intended for test-use only?

[akarnokd]

To test if the lockdown actually works, this has to be locked down but then unlocked to allow other tests setting their hooks after that.

[ZacSweers]

Could you include an example of what the output from AssemblyStackTraceException looks like, for posterity? Doc might be a bit overkill, but it would be nice to see what it looks like

[akarnokd]

In the IDE, this is what it looks like when watching the exception (its message):

rx.exceptions.AssemblyStackTraceException: Assembly trace:
 at rx.Observable.create(Observable.java:92)
 at rx.Observable.lift(Observable.java:239)
 at rx.Observable.map(Observable.java:6321)
 at rx.plugins.RxJavaHooksTest.createObservable(RxJavaHooksTest.java:31)
 at rx.plugins.RxJavaHooksTest.assemblyTrackingObservable(RxJavaHooksTest.java:45)
 at java.lang.reflect.Method.invoke(Method.java:498)
Original exception:

If a test fails, Eclipse's JUnit window can nicely jump to these sources.

[akarnokd]

If necessary, we can remove more lines (such as Observable.xxx()) but they might be helpful while diagnosing problems in RxJava itself.

[ZacSweers]

I think the trace is fine as it is. Good to see it can pick it up as sources that are jumpable as well. We've been using a sort of TaggedObserver pattern to help distinguish traces where something bubbles up from an OnErrorNotImplementedException and it looks like this could help a lot in tracing the actual assembly.

👍 for me as long as setting schedulers dynamically still works.

[akarnokd]

All the old hooks and reset still work as this delegates to those by default (their original unit tests still pass).

[ZacSweers]

Would it be possible to use this new API for the same effect though? (changing schedulers on the fly, mainly for testing purposes)

[akarnokd]

You can change the schedulers on the fly via setOnComputationScheduler(), etc.

[ZacSweers]

Sounds good 👍

[akarnokd]

Thanks for the reviews!