Incorporate feedback from #130 #138

ZacSweers · 2017-11-18T08:46:34Z

This PR incorporates feedback from #130, but it not currently complete yet. CC @akarnokd whenever you have time to review (no rush!)

Completed

AutoDisposePlugins.java#L36: Such lockdown support may not be desirable on Android. The same feature was aimed at server/J2EE container use of RxJava.

Mentioned in the issue, opted to keep this to match semantics of RxJava's plugins. We also operate on a large monorepo structure internally where this actually becomes useful to help catch downstream usages.

dependencies.gradle#L65: Using a pretty old RxJava 2 version.

Touched on in the issue, and after thinking on it I think I'm going to keep this as is until there's a new minor release or new API we want to target. Keeping the lower API makes it easier for consumers to adopt without imposing newer versions if they haven't been able to update yet.

AutoDisposingCompletableObserverImpl.java#L73: synchronized is pointless here.
AutoDisposingCompletableObserverImpl.java#L80: synchronized is pointless here. If the purpose was to mutually exclude with #L73 then it is not worth it. If lazySet wins then dispose(ref) will do nothing, if they race, both cases could win probabilistically.

bd22bc2

AutoDisposingCompletableObserver.java#L30: missing space in forintrospection.

caf904a

AutoDisposingObserver.java#L30: missing tailing s from purpose?

79e6dbf

AutoDispose.java#L169: class could be final.
AutoDispose.java#L199: class could be final.
AutoDispose.java#L229: class could be final.

94212eb

AutoDisposingCompletableObserverImpl.java#L45: The DisposableMaybeObserver could be created and setOnce'd before the call to lifecycle.subscribe().

9bf1993

AutoDisposingObserverImpl.java: Same problems as with the CompletableObserver implementation. Here one would need a HalfSerializer to mutually exclude the onXXX events (as there could be multiple onNext calls).

cb432a8. I think I've mostly got this. Main thing I'm not sure of is if it does terminate, do we need to dispose the lifecycle or does this mean that onError or onComplete have been hit in the main observer already (and doing so is a double dispose condition)

ViewAttachEventsObservable.java#L41 If the call is not on the main thread, onSubscribe is not called which can lead to a NullPointerException in the dowstream. The no-op Disposable should be sent to the Observer first.

07f0f1e and corresponding one for LifecycleEventsObservable ec190c0

ViewAttachEventsObservable.java#L51: The Observer may cancel immediately and since the listener is not registered with the view yet, it won't get removed but then added unconditionally. Assuming the events also come from the main thread, one could add the listener first, and call onSubscribe next. If adding the listener triggers an event emission immediately, keep the current order and after adding the listener, check if the listener has been disposed in the meantime and remove the listener.

I think this is covered as a part of 07f0f1e, but let me know if I've missed something

AutoDisposingSubscriberImpl.java#L80: There is no onStart provided by the API and the delegate.onSubscribe is called after mainSubscription is definitely set: the null check should always pass and thus unnecessary.

b38d03f

TODO

AutoDisposingCompletableObserverImpl.java#L52: The mainDisposable could be already set and the delegate currently executing one of its onXXX methods which violates the contract unless it is guaranteed both the lifecycle signal and the onXXX calls happen on the same thread (i.e., forced observeOn(AndroidSchedulers.mainThread())). Possible solution: always call delegate.onSubscribe(this) first, mutually exclude the onXXX signals via an AtomicBoolean once.

If I subscribe first - couldn't it synchronously execute its entire sequence before the lifecycle gets a chance then? Part of ths reason it's done ater the lifecycle check is such that the lifecycle can indicate that it's ended, and just immediately dispose after onSubscribe().

AutoDisposingCompletableObserverImpl.java#L58: May not be a good idea to leave the delegate non-terminated in this case, if the lifecycle actually can get onComplete.

Could you expand on this one a bit? I'm not sure I totally understood

AutoDisposingSubscriberImpl.java#L80: Note though that if one does the setOnce(mainSubscription) before the lifecycle is attached, one may need to use deferred requesting to not trigger the upstream emission while the code is setting up other things in onSubscribe.

See comment below

Adds a convenience assertion and uses peek instead of poll for hasError

These were missing before and necessary to mark this observer as appropriately disposed after, though without calling dispose()

Saving dex methods

I'm not 100% I've done this right, but leveraging some existing examples in RxJava as my guide. I've copied in the relevant internal rxjava APIs (since they're internal) and also adjusted the onNext methods to return true or false indicating whether or not they terminated the underlying observer. Will seek further feedback during the PR. Main thing I'm not sure of is if it does terminate, do we need to dispose the lifecycle or does this mean that onError or onComplete have been hit in the main observer already (and doing so is a double dispose condition)

ZacSweers · 2017-11-18T08:50:06Z

Could you expand on this one a bit? I haven't used flowable's APIs that much so I'm a little inexperienced with handling requesting or deferring it

I actually missed the link in your original comment. Going to research more with that and report back!

ZacSweers · 2017-11-18T10:48:36Z

Ok I think I've got deferred requesting set up in 500f337 after reading the docs. If that looks good, I'll bring those methods into the AutoDisposeSubscriptionHelper copy so it's not depending directly on internal classes

ZacSweers · 2017-11-26T09:28:42Z

Ok, I'm missing something about how to wire up deferred requesting. Let me know if you have any advice!

akarnokd · 2017-11-26T09:57:22Z

Here is the wiki entry for deferred requesting.

AutoDisposingCompletableObserverImpl.java#L58: May not be a good idea to leave the delegate non-terminated in this case, if the lifecycle actually can get onComplete.

Could you expand on this one a bit? I'm not sure I totally understood

Does the lifecycle's onComplete get called while there is still a subscriber active? Consider this:

PublishSubject.create()
.to(AutoDispose.with(ScopeProvider.UNBOUND).forObservable())
.subscribe(new DisposableObserver<Object>() {
    PrintWriter fout;

    @Override public void onStart() {
        fout = new PrintWriter(new FileWriter("output.txt"));
    }

    @Override public void onNext(Object o) {
        fout.println(o);
    }

    @Override public void onError(Throwable t) {
        t.printStackTrace(fout);
        fout.close();
    }

    @Override public void onComplete() {
        fout.close();
    }
});

If the lifecycle ends, fout may left open.

ZacSweers · 2017-11-26T11:27:42Z

Yep that's correct behavior. Something may be considered "unbound" in the sense that they won't be disposed by the lifecycle ending. In the example case, it would be as if it was running without autodisposing. The intention for ScopeProvider.UNBOUND is to be a shortcut for upstream providers to pass down. Doing it exactly as your snippet does would be considered sort of wasteful, in the same way takeUntil(Observable.never()) might be.

For the deferred requesting, I tried implementing pretty much like the Operator example in the wiki, but I get an error I don't quite understand when I try to test with it:

java.lang.AssertionError: Not subscribed! (latch = 1, values = 0, errors = 0, completions = 0)

	at io.reactivex.observers.BaseTestConsumer.fail(BaseTestConsumer.java:162)
	at io.reactivex.subscribers.TestSubscriber.assertSubscribed(TestSubscriber.java:309)
	at com.uber.autodispose.AutoDisposeSubscriberTest.autoDispose_withProvider(AutoDisposeSubscriberTest.java:124)

Does anything stand out to you as off in my implementation?

akarnokd · 2017-11-26T11:34:50Z

Could you point me to the implementation that is targeted by that test?

ZacSweers · 2017-11-26T11:36:21Z

Changes -
500f337

And the whole impl - https://github.com/uber/AutoDispose/blob/500f3373ebc891cf74512e15e9671393a539307f/autodispose/src/main/java/com/uber/autodispose/AutoDisposingSubscriberImpl.java

akarnokd · 2017-11-26T11:40:29Z

You have to call delegate.onSubscribe(this) before L74.

ZacSweers · 2017-11-26T12:24:42Z

That unfortunately causes several other tests to fail with a double subscription error:

java.lang.AssertionError: Error(s) present: [java.lang.IllegalStateException: onSubscribe received multiple subscriptions: EmptySubscription] (latch = 1, values = 0, errors = 1, completions = 0)
	at io.reactivex.observers.BaseTestConsumer.fail(BaseTestConsumer.java:162)
	at io.reactivex.observers.BaseTestConsumer.assertNoErrors(BaseTestConsumer.java:247)
	at com.uber.autodispose.AutoDisposeSubscriberTest.autoDispose_withScopeProviderCompleted_shouldNotReportDoubleSubscriptions(AutoDisposeSubscriberTest.java:347)

java.lang.AssertionError: Error(s) present: [java.lang.IllegalStateException: onSubscribe received multiple subscriptions: EmptySubscription] (latch = 1, values = 0, errors = 1, completions = 0)
	at io.reactivex.observers.BaseTestConsumer.fail(BaseTestConsumer.java:162)
	at io.reactivex.observers.BaseTestConsumer.assertNoErrors(BaseTestConsumer.java:247)
	at com.uber.autodispose.AutoDisposeSubscriberTest.autoDispose_withProviderAndNoOpPlugin_afterEnding_shouldFailSilently(AutoDisposeSubscriberTest.java:247)

Specifically -

AutoDisposeSubscriberTest. autoDispose_withProviderAndNoOpPlugin_afterEnding_shouldFailSilently
AutoDisposeSubscriberTest. autoDispose_withProviderAndNoOpPlugin_withoutStarting_shouldFailSilently
AutoDisposeSubscriberTest. autoDispose_withProviderAndPlugin_withoutStarting_shouldFailWithExp
AutoDisposeSubscriberTest. autoDispose_withProvider_afterLifecycle_shouldFail
AutoDisposeSubscriberTest. autoDispose_withProvider_withoutStartingLifecycle_shouldFail
AutoDisposeSubscriberTest. autoDispose_withScopeProviderCompleted_shouldNotReportDoubleSubscriptions

ZacSweers · 2017-11-26T12:42:20Z

Also - calling delegate.onSubscribe() before the lifecycle kind of gets to the other remaining concern I had in the description:

If I subscribe first - couldn't it synchronously execute its entire sequence before the lifecycle gets a chance then? Part of the reason it's done after the lifecycle check is such that the lifecycle can indicate that it's ended, and just immediately dispose after onSubscribe().

Would be curious to know your thoughts

akarnokd · 2017-11-26T13:17:07Z

You don't need to call callMainSubscribeIfNecessary as the delegate's onSubscribe will be called before the lifecycle is subscribed.

ZacSweers · 2017-11-27T00:49:44Z

D'oh, I totally missed that. Fixed in e7e1b87. With that, I think I've covered the points from your issue if you want to do a full review?

akarnokd · 2017-11-27T08:58:40Z

...mponents/src/main/java/com/uber/autodispose/android/lifecycle/LifecycleEventsObservable.java

    if (!isMainThread()) {
      observer.onError(
          new IllegalStateException("Lifecycles can only be bound to on the main thread!"));
      return;
    }
-    ArchLifecycleObserver archObserver =
-        new ArchLifecycleObserver(lifecycle, observer, eventsObservable);
-    observer.onSubscribe(archObserver);
    lifecycle.addObserver(archObserver);


If the Observer disposes immediately, this will still add the listener and keep a reference to it. You should check for isDisposed after this add and call removeObserver:

lifecycle.addObserver(archObserver); if (archObserver.isDisposed()) { lifecycle.removeObserver(); }

akarnokd · 2017-11-27T08:59:25Z

...todispose-android/src/main/java/com/uber/autodispose/android/ViewAttachEventsObservable.java

@@ -46,8 +48,6 @@
      // Emit the last event, like a behavior subject
      observer.onNext(ViewLifecycleEvent.ATTACH);
    }
-    Listener listener = new Listener(view, observer);
-    observer.onSubscribe(listener);
    view.addOnAttachStateChangeListener(listener);


Here too, if disposed, the listener will remain added.

akarnokd · 2017-11-27T09:00:12Z

autodispose/src/main/java/com/uber/autodispose/AutoDispose.java

@@ -166,7 +166,7 @@
    return new LifecycleScopeProviderHandlerImpl(scope);
  }

-  private static class MaybeScopeHandlerImpl implements ScopeHandler {
+  private static final class MaybeScopeHandlerImpl implements ScopeHandler {


I'd avoid private in case you don't want to get accessor methods on Android.

Good catch. Fixed in 959fe09 but planning to remove this class in the near future anyway now that as() is in RxJava :)

akarnokd · 2017-11-27T09:01:58Z

autodispose/src/main/java/com/uber/autodispose/AutoDisposingCompletableObserverImpl.java

+      }
+    };
+    if (AutoDisposeEndConsumerHelper.setOnce(lifecycleDisposable, o, getClass())) {
+      lifecycle.subscribe(o);
      if (AutoDisposeEndConsumerHelper.setOnce(mainDisposable, d, getClass())) {
        delegate.onSubscribe(this);


callMainSubscribeIfNecessary may be a problem here too causing double onSubscribes.

Hmm, the only events that could cause this though would be if the lifecycle terminated, in which case mainDisposable would either be dispose()'d or be lazySet to DISPOSED (and thus avoid a double subscription error from being thrown) right? Or do you mean there's race condition potential if the lifecycle terminates in the middle of this code execution?

Unless you can confine this entire onSubscribe to the main thread, it is possible this gets executed asynchronously while the main thread ends the lifecycle.

Actually I think I see what you mean. I can call delegate.onSubscribe early rather than try to be defensive against lifecycle ending immediately (which is what I keep being worried about, as it bit me in the past). We're already subscribed at this point so it's not like calling onSubscribe on the delegate early is going to change anything

Done in ba8bd87

akarnokd · 2017-11-27T09:02:51Z

autodispose/src/main/java/com/uber/autodispose/AutoDisposingCompletableObserverImpl.java

      delegate.onComplete();
    }
  }

  @Override public void onError(Throwable e) {
    if (!isDisposed()) {
-      lazyDispose();
+      AutoDisposableHelper.dispose(lifecycleDisposable);
+      mainDisposable.lazySet(AutoDisposableHelper.DISPOSED);


Swap these so the mainDisposable change gets flushed with the dispose() call.

As in just swap the order they're executed?

mainDisposable.lazySet(AutoDisposableHelper.DISPOSED); AutoDisposableHelper.dispose(lifecycleDisposable);

Assuming so after googling a bit - part of 9c3edf4

akarnokd · 2017-11-27T09:03:41Z

autodispose/src/main/java/com/uber/autodispose/AutoDisposingMaybeObserverImpl.java

+      }
+    };
+    if (AutoDisposeEndConsumerHelper.setOnce(lifecycleDisposable, o, getClass())) {
+      lifecycle.subscribe(o);
      if (AutoDisposeEndConsumerHelper.setOnce(mainDisposable, d, getClass())) {
        delegate.onSubscribe(this);


callMainSubscribeIfNecessary may cause double onSubscribe here.

part of ba8bd87

akarnokd · 2017-11-27T09:03:55Z

autodispose/src/main/java/com/uber/autodispose/AutoDisposingMaybeObserverImpl.java

@@ -95,21 +91,24 @@ void callMainSubscribeIfNecessary(Disposable d) {

  @Override public void onSuccess(T value) {
    if (!isDisposed()) {
-      lazyDispose();
+      AutoDisposableHelper.dispose(lifecycleDisposable);
+      mainDisposable.lazySet(AutoDisposableHelper.DISPOSED);


Swap these.

part of 9c3edf4

akarnokd · 2017-11-27T09:04:19Z

autodispose/src/main/java/com/uber/autodispose/AutoDisposingObserverImpl.java

+      }
+    };
+    if (AutoDisposeEndConsumerHelper.setOnce(lifecycleDisposable, o, getClass())) {
+      lifecycle.subscribe(o);
      if (AutoDisposeEndConsumerHelper.setOnce(mainDisposable, d, getClass())) {
        delegate.onSubscribe(this);


Double onSubscribe.

part of ba8bd87

akarnokd · 2017-11-27T09:04:49Z

autodispose/src/main/java/com/uber/autodispose/AutoDisposingSingleObserverImpl.java

+      }
+    };
+    if (AutoDisposeEndConsumerHelper.setOnce(lifecycleDisposable, o, getClass())) {
+      lifecycle.subscribe(o);
      if (AutoDisposeEndConsumerHelper.setOnce(mainDisposable, d, getClass())) {
        delegate.onSubscribe(this);


Double onSubscribe.

part of ba8bd87

We were disposing the lifecycle a second time after, and unnecessarily disposing the main disposable twice too

ZacSweers · 2017-11-27T12:36:05Z

I think I've addressed the existing comments and a little extra. Thanks again for taking the time to review this, I'm learning a lot!

ZacSweers · 2017-12-03T00:53:01Z

@akarnokd let me know if there's anything else you see, otherwise I'll try to land this monday or tuesday

akarnokd · 2017-12-03T09:04:12Z

No further comments. 👍

ZacSweers · 2017-12-03T23:56:13Z

Awesome. Thank you so much for the review!

ZacSweers added 14 commits November 17, 2017 22:50

Remove unnecessary synchronized blocks

bd22bc2

Fix introspection typo

caf904a

Fix introspection typo in observer

79e6dbf

Make AutoDispose ScopeHandlers final

94212eb

Extract DisposableMaybeObserver and setOnce first

9bf1993

Opportunistic tweak of RxErrorsRule

53eef69

Adds a convenience assertion and uses peek instead of poll for hasError

Lazyset disposables in scope callbacks to prevent double subscriptions

7ac1488

These were missing before and necessary to mark this observer as appropriately disposed after, though without calling dispose()

Inline lazyDispose() methods

d564b40

Saving dex methods

Fix ViewAttachEventsObservable not observing observer contract

07f0f1e

Fix LifecycleEventsObservable not observing observer contract

ec190c0

Inline lazyDispose() impl

9606ae4

Remove unnecessary nullcheck in request()

b38d03f

Suppress nullaway false positive

7705288

ZacSweers added Android Documentation Enhancement labels Nov 18, 2017

ZacSweers added this to the 0.5.0 milestone Nov 18, 2017

ZacSweers self-assigned this Nov 18, 2017

ZacSweers mentioned this pull request Nov 18, 2017

Fix a racing condition when subscribing to a completed ScopeProvider. #135

Closed

ZacSweers added 2 commits November 18, 2017 02:43

Initialize atomic throwables inline

daad41b

Use deferred requesting

500f337

Call delegate.onSubscribe() first, remove callSubscribeIfNecessary

e7e1b87

ZacSweers requested a review from tonycosentini November 27, 2017 00:52

akarnokd reviewed Nov 27, 2017

View reviewed changes

ZacSweers added 8 commits November 27, 2017 03:44

Check if observer is disposed in lifecycle events

a0d8556

Check if listener is disposed in viewattacheventsobservable

22ba683

Remove private to avoid synthetic accessor

959fe09

Use non-rx-internal subscriptionhelper APIs

6a45aa4

Opportunistic fix missing synchronized in overridden method

44b362f

Remove callSubscribeIfNecessary, avoid duplicate subscribes

ba8bd87

Switch orders of lazySet + dispose to flush

9c3edf4

Optimize lifecycle error propagation a bit

3b87876

We were disposing the lifecycle a second time after, and unnecessarily disposing the main disposable twice too

akarnokd approved these changes Dec 3, 2017

View reviewed changes

ZacSweers merged commit 16a517f into master Dec 3, 2017

ZacSweers deleted the z/feedback branch December 3, 2017 23:56

This was referenced Dec 3, 2017

Review of RxJava 2 usage in the project #130

Closed

Release notes for 0.5.0 #129

Closed

Incorporate feedback from #130 #138

Incorporate feedback from #130 #138

Conversation

ZacSweers commented Nov 18, 2017 • edited Loading

Completed

TODO

ZacSweers commented Nov 18, 2017

ZacSweers commented Nov 18, 2017

ZacSweers commented Nov 26, 2017

akarnokd commented Nov 26, 2017

ZacSweers commented Nov 26, 2017 • edited Loading

akarnokd commented Nov 26, 2017

ZacSweers commented Nov 26, 2017

akarnokd commented Nov 26, 2017

ZacSweers commented Nov 26, 2017 • edited Loading

ZacSweers commented Nov 26, 2017

akarnokd commented Nov 26, 2017

ZacSweers commented Nov 27, 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

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

ZacSweers commented Nov 27, 2017

ZacSweers commented Dec 3, 2017

akarnokd commented Dec 3, 2017

ZacSweers commented Dec 3, 2017

ZacSweers commented Nov 18, 2017 •

edited

Loading

ZacSweers commented Nov 26, 2017 •

edited

Loading

ZacSweers commented Nov 26, 2017 •

edited

Loading