Review of RxJava 2 usage in the project #130
Comments
|
Thank you so much David! I'll get on these today. Would you mind if I added you as a reviewer on the subsequent PR(s)? |
|
Sure. |
|
Working on a branch and had a few questions/comments.
Is this due to using
True, but this was built to be a Java library first with android extensions :). Mostly added to keep the semantics familiar to RxJava.
Good point. I tend to prefer making extension libraries depend on lower versions of libraries to avoid imposing burden on consumers that maybe haven't been able to update yet (we hit this often). I suppose it could also become a liability if new changes caused unknown behavior changes here. I'll think on this more.
I should elaborate that comment in source a bit. The behavior is such that if the scope "completes", then the binding is considered "unbound" and free to execute to termination. The idea is that scopes might sometimes be required for an API, but consumers of that API may want to just indicate that they can be unbound. They may dynamically resolve to be unbound under certain conditions at runtime too, and I wanted to support making that explicit. Otherwise scopes could just be defined as |
The problem is that this subscription happens before the associates
Risky. It would leave the consumer that is interested in handling |
|
Could you expand on this bit some? I'm not sure I follow
Particularly, the terminal emissions of the scope emissions are not visible outside of the autodisposing observer implementations, so I'm not sure which consumer would be interested in handling it. If the lifecycle |
|
I'm sorry I have to postpone further explanations till next week as I have an important submission deadline coming up. |
|
No problem at all! |
* Remove unnecessary synchronized blocks * Fix introspection typo * Fix introspection typo in observer * Make AutoDispose ScopeHandlers final * Extract DisposableMaybeObserver and setOnce first * Opportunistic tweak of RxErrorsRule Adds a convenience assertion and uses peek instead of poll for hasError * Lazyset disposables in scope callbacks to prevent double subscriptions These were missing before and necessary to mark this observer as appropriately disposed after, though without calling dispose() * Inline lazyDispose() methods Saving dex methods * Wire up HalfSerializer to observer and subscribers 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) * Fix ViewAttachEventsObservable not observing observer contract * Fix LifecycleEventsObservable not observing observer contract * Inline lazyDispose() impl * Remove unnecessary nullcheck in request() * Suppress nullaway false positive * Initialize atomic throwables inline * Use deferred requesting * Call delegate.onSubscribe() first, remove callSubscribeIfNecessary * Check if observer is disposed in lifecycle events * Check if listener is disposed in viewattacheventsobservable * Remove private to avoid synthetic accessor * Use non-rx-internal subscriptionhelper APIs * Opportunistic fix missing synchronized in overridden method * Remove callSubscribeIfNecessary, avoid duplicate subscribes * Switch orders of lazySet + dispose to flush * Optimize lifecycle error propagation a bit We were disposing the lifecycle a second time after, and unnecessarily disposing the main disposable twice too
|
Done in #138 |
Hi. I was asked to review the library's RxJava 2 usage for potential logical or concurrency issues.
Review @ commit: 0147484b
Main issues:
DisposableMaybeObservercould be created andsetOnce'd before the call tolifecycle.subscribe().mainDisposablecould be already set and thedelegatecurrently executing one of itsonXXXmethods which violates the contract unless it is guaranteed both the lifecycle signal and the onXXX calls happen on the same thread (i.e., forcedobserveOn(AndroidSchedulers.mainThread())). Possible solution: always calldelegate.onSubscribe(this)first, mutually exclude the onXXX signals via anAtomicBoolean once.onComplete.synchronizedis pointless here.synchronizedis pointless here. If the purpose was to mutually exclude with#L73then it is not worth it. IflazySetwins thendispose(ref)will do nothing, if they race, both cases could win probabilistically.CompletableObserverimplementation.CompletableObserverimplementation. Here one would need aHalfSerializerto mutually exclude the onXXX events (as there could be multipleonNextcalls).CompletableObserverimplementation.CompletableObserverimplementation and then some.onStartprovided by the API and thedelegate.onSubscribeis called aftermainSubscriptionis definitely set: the null check should always pass and thus unnecessary. Note though that if one does thesetOnce(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 inonSubscribe.onSubscribeis not called which can lead to aNullPointerExceptionin the dowstream. The no-opDisposableshould be sent to theObserverfirst.Observermay cancel immediately and since the listener is not registered with theviewyet, 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 callonSubscribenext. 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.ViewAttachEventsObservable.Minor issues:
forintrospection.sfrompurpose?final.final.final.The text was updated successfully, but these errors were encountered: