Implement LambdaConsumerIntrospection by ZacSweers · Pull Request #5590 · ReactiveX/RxJava

ZacSweers · 2017-09-06T22:41:08Z

Followup from #5569, and allows you to introspect if the resulting observer has missing error consumption and subsequently supplies a default (throwing) one.

Followup from ReactiveX#5569, and allows you to introspect if the resulting observer has missing error consumption and subsequently supplies a default (throwing) one.

ZacSweers · 2017-09-06T22:42:01Z

Wasn't sure how you'd want to do the naming. Can work on adding some tests if this looks good, wanted to get something up for API review early

akarnokd · 2017-09-06T22:47:21Z

src/main/java/io/reactivex/observers/HasDefaultErrorConsumer.java

+
+/**
+ * An interface that indicates that the implementing type has default implementations for error consumption.
+ */


@since 2.1.4 - experimental

akarnokd · 2017-09-06T22:48:21Z

src/main/java/io/reactivex/internal/observers/LambdaObserver.java

    }
+
+    @Override
+    public boolean hasMissingErrorConsumer() {


A test would be great.

akarnokd · 2017-09-06T22:49:07Z

The naming is fine with me.

codecov · 2017-09-06T23:01:47Z

Codecov Report

Merging #5590 into 2.x will increase coverage by 0.08%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##                2.x    #5590      +/-   ##
============================================
+ Coverage     96.15%   96.23%   +0.08%     
- Complexity     5827     5836       +9     
============================================
  Files           632      632              
  Lines         41459    41465       +6     
  Branches       5742     5742              
============================================
+ Hits          39863    39904      +41     
+ Misses          638      621      -17     
+ Partials        958      940      -18

Impacted Files	Coverage Δ	Complexity Δ
...nternal/observers/CallbackCompletableObserver.java	`100% <100%> (ø)`	`11 <2> (+2)`	⬆️
...o/reactivex/internal/observers/LambdaObserver.java	`100% <100%> (ø)`	`14 <2> (+2)`	⬆️
...nternal/operators/maybe/MaybeCallbackObserver.java	`100% <100%> (ø)`	`9 <2> (+2)`	⬆️
...vex/internal/observers/ConsumerSingleObserver.java	`100% <100%> (ø)`	`9 <2> (+2)`	⬆️
...x/internal/observers/EmptyCompletableObserver.java	`100% <100%> (ø)`	`8 <1> (+1)`	⬆️
...activex/internal/subscribers/LambdaSubscriber.java	`100% <100%> (ø)`	`16 <2> (+2)`	⬆️
...a/io/reactivex/internal/util/QueueDrainHelper.java	`58.86% <0%> (-4.26%)`	`31% <0%> (-3%)`
.../internal/operators/flowable/FlowableInterval.java	`94.44% <0%> (-2.78%)`	`3% <0%> (ø)`
...activex/internal/observers/QueueDrainObserver.java	`61.53% <0%> (-2.57%)`	`12% <0%> (-1%)`
.../io/reactivex/disposables/CompositeDisposable.java	`97.24% <0%> (-1.84%)`	`39% <0%> (-1%)`
... and 27 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 c43229b...500df33. Read the comment docs.

ZacSweers · 2017-09-06T23:04:44Z

Should I add this to other types' observers too? Or do you want to start with this first?

akarnokd · 2017-09-07T07:34:38Z

Should I add this to other types' observers too?

That would be great!

artem-zinnatullin · 2017-09-07T08:34:01Z

Btw @hzsweers, you can solve this right now by overriding (via reflection) io.reactivex.internal.functions.Functions#ON_ERROR_MISSING with Consumer<Throwable> implementation that'll throw error instead of delivering it to the RxJavaPlugins

Although current WIP looks fine for the most part 👍

ZacSweers · 2017-09-07T23:21:12Z

yeah I'd prefer something in the public API :).

@akarnokd should there be a separate HasDefaultErrorConsumer for LambdaSubscriber for symmetry? Or just reuse?

ZacSweers · 2017-09-07T23:50:04Z

Just reused for now, let me know if you want me to make it separate.

I think I've covered the other observer possibilities, let me know if there's any others I should cover

artem-zinnatullin · 2017-09-08T01:36:15Z

src/main/java/io/reactivex/observers/HasDefaultErrorConsumer.java

+ * @since 2.1.4 - experimental
+ */
+@Experimental
+public interface HasDefaultErrorConsumer {


I think we should find a better name for this interface for several reasons:

HasDefaultErrorConsumer sounds like marker interface that you need to use with instanceof to get boolean result.

Interface HasDefaultErrorConsumer and method name hasMissingErrorConsumer() are inconsistent.

Extensibility — do we plan to extend it with other functions to check onNext/onComplete implementations?

I'd definitely get rid of "default" and "missing" from both. The negated method is really confusing. There's already Has* types which do similar things so I don't agree that it needs changed.

Hm, last sentence dropped from my comment for some reason.

Depending on answer to №3 I have following interface names on my mind ObserverIntrospection/SubscriberIntrospection or ErrorConsumerIntrospection, hope someone has better suggestions :)

The negated method is really confusing.

Huh, indeed! Didn't notice that until you pointed

I let you people work out the naming.

for the method, how about just a simple isNotImplemented() or isThrowing()?

Assuming the interface itself gets named HasErrorConsumer?

People will most likely mistreat CompositeObserver and think of it as of a combination of observers because that's how CompositeDisposable and CompositeException work.

ObserverInstrumentation/ObserverIntrospection? Such a name also more clearly delivers the intention of consume-only public API

Composite is misleading and will show up in content assist when the developer types Composite

to get to the disposable/exception variant mentioned.

I'd call it LambdaConsumerIntrospection and hasCustomOnError()

Good points. The Introspection moniker feels off to me, mostly because I consider introspection an act and don't necessarily think of it as a noun/type at first. I don't know what a better word would be though. Updated in bae194c

artem-zinnatullin · 2017-09-08T01:38:30Z

src/test/java/io/reactivex/internal/observers/CallbackCompletableObserverTest.java

+    }
+
+    @Test
+    public void isNotMissingErrorConsumer() {


nit: doesNotHaveMissingErrorConsumer?

Made them clearer in 500df33

ZacSweers · 2017-09-08T09:19:57Z

I agree the names could be better, will update tomorrow

ZacSweers · 2017-09-10T01:11:40Z

Ok I've updated the tests and the naming based on CR. The only remaining bit I think is whether or not to use a global introspection interface or make one per observer for better parity. Thoughts?

artem-zinnatullin · 2017-09-10T02:12:59Z

use a global introspection interface or make one per observer for better parity. Thoughts?

Depends on which level of per observer you're talking about. Hopefully, not about all internal implementations :)

I'd create an interface per Rx-type, so you could hook into these RxJavaPlugins callbacks:

ie: MaybeObserverIntrospection, FlowableSubscriberIntrospection, SingleObserverIntrospection, ObservableObserverIntrospection, CompletableObserverIntrospection.

Because if we'll decide to add introspection API for other callbacks like onNext and onComplete in future, some of them will not make sense for types like Completable and Single and naming won't match their callbacks if we go with same interface for all types.

LambdaConsumerIntrospection seems too impl-specific name, I realise that the only way RxJava can understand that ie error handler is default one is to rely on subscribe() calls that accept lambdas. But name still feels kinda wrong and too specific. Just my 2 satoshis.

ZacSweers · 2017-09-10T05:58:34Z

Yeah I meant per-type. I think the name is fine as a specific thing since it's only applicable to composed observers like lambdaobserver. Lambda itself might not be best since that naming convention seems to only exist for Observable and Flowable

akarnokd · 2017-09-10T09:36:53Z

I think there is no need to have 5 versions of the same interface; there is always the Subscriber/Observer type discoverable:

public static void errorNotImplemented(Object target) {
    if (target instanceof LambdaConsumerIntrospection) {
        if (target instanceof SingleObserver) {
            // ...
        }
        if (target instanceof Subscriber) {
            // ...
        }
    }
}

ZacSweers · 2017-09-10T21:44:37Z

that works for me. I think this PR is complete then

artem-zinnatullin · 2017-09-11T00:13:20Z

src/main/java/io/reactivex/observers/LambdaConsumerIntrospection.java

+     * implementation is missing an error consumer and thus using a throwing default implementation.
+     */
+    @Experimental
+    boolean hasCustomOnError();


Shouldn't we provide hasDefaultOnError() instead? cc @JakeWharton

Because if you think about it from RxJava perspective — that's what library can say about itself: is something has a default value/implementation in compare to what library thinks is default.

hasCustomOnError() basically checks for hasDefaultOnError() and then negates the result which is kinda confusing.

TL;TR: "custom" in the API feels wrong, but maybe it's just me :)

artem-zinnatullin · 2017-09-11T00:16:38Z

@akarnokd questionable decision about types, Single and Completable might be inconsistent with future addition of hasDefaultOnNext() and hasDefaultOnComplete() changes in the interface.

up2u of course

akarnokd · 2017-09-11T07:37:55Z

If you want to hijack those consumer types, you already know the target type and which signal types it can deliver. Therefore, if they implement the same broad interface, the impossible methods can return false or throw an UnsupportedOperationException.

Implement HasDefaultErrorConsumer

da4e67f

Followup from ReactiveX#5569, and allows you to introspect if the resulting observer has missing error consumption and subsequently supplies a default (throwing) one.

ZacSweers mentioned this pull request Sep 6, 2017

2.x: Strawman proposal for uncaught exception configuration #5569

Closed

akarnokd reviewed Sep 6, 2017

View reviewed changes

akarnokd added 2.x Enhancement labels Sep 6, 2017

akarnokd added this to the 2.2 milestone Sep 6, 2017

ZacSweers added 2 commits September 6, 2017 15:51

Add @since

ded2448

Add tests

3f15c24

ZacSweers mentioned this pull request Sep 6, 2017

Expose the underlying observer in AutoDisposeObservers uber/AutoDispose#78

Closed

ZacSweers added 4 commits September 7, 2017 16:42

Add support in relevant completable observers

e3882c6

Add support in ConsumerSingleObserver

b4f1509

Add support in MaybeCallbackObserverTest

8d491eb

Add support in LambdaSubscriber

4555b47

artem-zinnatullin reviewed Sep 8, 2017

View reviewed changes

ZacSweers added 3 commits September 8, 2017 19:14

Switch to CompositeObserver and onErrorImplemented()

e2d06c0

Update wording to use Introspection

bae194c

Update tests and flip implementation logic to match naming

500df33

akarnokd approved these changes Sep 10, 2017

View reviewed changes

ZacSweers changed the title ~~Implement HasDefaultErrorConsumer~~ Implement LambdaConsumerIntrospection Sep 10, 2017

artem-zinnatullin reviewed Sep 11, 2017

View reviewed changes

akarnokd merged commit 99a620a into ReactiveX:2.x Sep 12, 2017

akarnokd mentioned this pull request Sep 18, 2017

2.1.4 release preparation #5607

Closed

mswysocki mentioned this pull request Sep 21, 2017

Add support for exposing delegate observers. uber/AutoDispose#89

Merged

jared-bot mentioned this pull request Sep 27, 2017

Dependency update: rxjava xmartlabs/bigbang-template#14

Merged

nikitin-da mentioned this pull request Oct 4, 2017

RxJava 2 support pushtorefresh/storio#685

Closed

ZacSweers mentioned this pull request Nov 9, 2017

Add subscribeBy extensions for proxy classes. uber/AutoDispose#127

Merged

ajalt mentioned this pull request Nov 9, 2017

Support LambdaConsumerIntrospection in subscribeBy. ReactiveX/RxKotlin#151

Merged

akarnokd mentioned this pull request May 27, 2018

2.x: Uncaught errors fail silently in junit tests. #5234

Closed

ZacSweers deleted the z/hasDefaultErrorConsumer branch May 27, 2018 00:04

akarnokd mentioned this pull request Jan 6, 2021

2.x, 3.x: Feature for detecting leaked subscriptions #7142

Closed

Conversation

ZacSweers commented Sep 6, 2017

Uh oh!

ZacSweers commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akarnokd commented Sep 6, 2017

Uh oh!

codecov bot commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ZacSweers commented Sep 6, 2017

Uh oh!

akarnokd commented Sep 7, 2017

Uh oh!

artem-zinnatullin commented Sep 7, 2017

Uh oh!

ZacSweers commented Sep 7, 2017

Uh oh!

ZacSweers commented Sep 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZacSweers Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZacSweers commented Sep 8, 2017

Uh oh!

ZacSweers commented Sep 10, 2017

Uh oh!

artem-zinnatullin commented Sep 10, 2017

Uh oh!

ZacSweers commented Sep 10, 2017

Uh oh!

akarnokd commented Sep 10, 2017

Uh oh!

ZacSweers commented Sep 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artem-zinnatullin commented Sep 11, 2017

Uh oh!

akarnokd commented Sep 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov bot commented Sep 6, 2017 •

edited

Loading

ZacSweers Sep 8, 2017 •

edited

Loading