Async #147
Async #147
Conversation
mvicore-android/src/main/java/com/badoo/mvicore/android/MVICoreSchedulers.kt
Outdated
Show resolved
Hide resolved
mvicore-android/src/main/java/com/badoo/mvicore/android/MVICoreSchedulers.kt
Outdated
Show resolved
Hide resolved
| newsPublisher: NewsPublisher<Action, Effect, State, News>? = null, | ||
| featureScheduler: Scheduler? = null, | ||
| private val observationScheduler: Scheduler? = null | ||
| ) : AsyncFeature<Wish, State, News> { |
There was a problem hiding this comment.
BaseFeature now implements the AsyncFeature, which implies that it is async. However it is not always async, as the behaviour depends on whether the schedulers are passed or not. Maybe introduce a separate BaseAsyncFeature which will require schedulers?
There was a problem hiding this comment.
In this case we need carefully support 2 almost the same features, most of the code will be just copy-pasted to new BaseAsyncFeature.
There was a problem hiding this comment.
Yes. You can try extracting/delegating things. From my point of view cleaner API is more important than implementation details boilerplate. Also with the current approach it is possible violates the Liskov Substitution principle, as you can't freely replace one AsyncFeature with another, and one Feature with another. They may behave differently.
| disposables += | ||
| actionSubject | ||
| .observeOnNullable(featureScheduler) | ||
| .subscribe { invokeActor(state, it) } |
There was a problem hiding this comment.
Just FYI, switching threads when invoking the Actor and/or invoking the Reducer maybe dangerous and can lead to hard-to-debug bugs. E.g. when disabling a view on button click, when done asynchronously, will allow the user to click the button multiple times. Also dispatching states/news synchronously allows other components to process them in a blocking manner. Switching threads will queue all the actions, and so in the feature you will not receive updates synchronously. All these race conditions are error-prone and not obvious.
We should proceed with care.
There was a problem hiding this comment.
You are right, that is why it won't be a recommendation to use it across the company and will be used to fix particular issue.
| private class ActorWrapper<State : Any, Action : Any, Effect: Any>( | ||
| private val threadVerifier: SameThreadVerifier, | ||
| private val disposables: DisposableCollection, | ||
| private operator fun CompositeDisposable.plusAssign(any: Any?) { |
There was a problem hiding this comment.
Should we raise an exception if this is not a disposable?
There was a problem hiding this comment.
I am a little worried to add it here, never checked how it actually works.
The current implementation is copied from DisposableCollection which I replaced with build-in CompositeDisposable. StandaloneMiddleware is Disposable, but other middlewares might not be. I am not ready to address this issue in this PR.
| if (disposables.isDisposed) return | ||
| val state = stateSubject.value!! |
There was a problem hiding this comment.
Add a description explaining what happened in case this throws a npe?
There was a problem hiding this comment.
Replaced with requireNotNull, but it can't be null because we initialise BehaviourSubject with initial value.
| .subscribe { effect -> invokeReducer(action, effect) } | ||
| ) | ||
| // Disposable might be disposed instantly in case of Observable.just | ||
| disposable.get()?.takeIf { !it.isDisposed }?.addTo(disposables) |
There was a problem hiding this comment.
Could we wrap the actor invoke in a defer to avoid the Observable just causing this problem ?
There was a problem hiding this comment.
Observable.defer { Observable.just } without subscribeOn is the same as just Observable.just.
I do not want to add subscribeOn here because we are already on featureScheduler and subscribeOn here will lead to delays and crippled stacktrace.
Made code a little bit better anyway.
| internal fun <T> Observable<T>.subscribeOnNullable(scheduler: Scheduler?): Observable<T> = | ||
| if (scheduler != null) subscribeOn(scheduler) else this | ||
|
|
||
| internal fun <T> Subject<T>.serializeIfNotNull(param: Any?): Subject<T> = |
There was a problem hiding this comment.
Nitpicking: the overhead of serialization should be negligible, so I would just always serialize to make the code simpler. Also I could not find any places where subjects may be called concurrently, the serialization seems unnecessary, unless I'm missing something.
There was a problem hiding this comment.
The subjects are exposed externally, it's possible that a developer could see unexpected behaviours if they subscribe on a different thread as the state is emitted.
Though I agree that perhaps this issue already exists, and making it always serialised might be safer
There was a problem hiding this comment.
Is not it possible to get a race condition during subscription (any thread) and emission (feature thread) at the same time?
There was a problem hiding this comment.
I assume that subscription should be always thread safe, but better to double check the implementation details. As far as I know, the serialization only prevents race conditions when sending events to subjects. E.g. the subject can call subscribers concurrently when onNext is called concurrently from different threads. Serialization prevents this at least.
# Conflicts: # build.gradle # mvicore/build.gradle # mvicore/src/test/java/com/badoo/mvicore/newspublishing/NewsPublishingTest.kt
No description provided.