Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rx: StorIOContentResolver updates running on StorIOContentResolverNotificationsThread #786

Open
clackbib opened this issue May 10, 2017 · 7 comments
Assignees

Comments

@clackbib
Copy link

Hello,
I've noticed that when using the StorIOContentResolver API, all emissions triggered by a contentResolver notifications come back on the same thread, so doing a long running operation on the same observable can delay notifications being received by another listener. For instance:

PreparedGetListOfObjects<> query = storIOSQLite
                .get()
                .listOfObjects(Tweet.class)
                .withQuery(TweetsTable.query)
                .prepare()

Subscription subA = query.asRxObservable()
         .doOnNext(tweets -> Log.d("TEST", "Logs immediately!"))
         .flatMap(tweets -> lonRunningObservable()) /* Say this runs for 10 seconds */
         .subscribe()


Subscription subB = query.asRxObservable()
         /* Delayed if notification was delivered to subscriber A*/
         .doOnNext(tweets -> Log.d("TEST", "Sometimes logs 10 seconds late!")) 
         .subscribe()
...

final List<Tweet> tweets = new ArrayList<Tweet>();
tweets.add(Tweet.newTweet("artem_zin", "Cool Tweet"));
storIOSQLite
     .put()
     .objects(tweets)
     .prepare()
     .asRxObservable()
     .subscribe();

I realize that it can be avoided by switching to another thread with observeOn(), but aren't updates supposed to use the default (or specified) scheduler?

@artem-zinnatullin
Copy link
Member

Default or specified scheduler for whole StorIOSQLite/StorIOContentResolver only used for initial subscription to the observable, so initial db request will go on that scheduler.

However further updates will be processed on thread that caused them, this is regular practice for rx related code and libraries and as a user you're able to change that with observeOn.

Moreover, if you know that operation is going to take a while it's pretty good idea to explicitly conrol thread it'll run on :)

However we can introduce some kind of defaultObserveOnScheduler to the library if there are will be enough requests from users. cc @nikitin-da thoughts?

@nikitin-da
Copy link
Collaborator

Hi!
I think it make sense in a greater degree for content resolver because of common StorIOContentResolverNotificationsThread.
But we will need symmetric api for sqlite too.
And we should deprecate current defaultScheduler method and rename it to defaultSubscribeOnScheduler

@artem-zinnatullin
Copy link
Member

@nikitin-da SGTM!

@artem-zinnatullin
Copy link
Member

@geralt-encore what do you think about proposal?

@geralt-encore
Copy link
Collaborator

I don't have deep knowledge about this topic but as far as I understand the proposal makes sense.

@clackbib
Copy link
Author

clackbib commented May 19, 2017

defaultSubscribeOnScheduler makes sense since fetching the data is always an I/O operation, defaultObserveOnScheduler maybe not completely? Nice to have a default, but since we can't assume the nature of the work that is done after the data returns (iO/ Computation/Main thread or other), the default may not be so helpful. How about enforcing the re-emission thread via a method param?
asRxObservable(Scheduler)
Either that, or internally moving the ContentObserver emission to the i/o pool?

@nikitin-da
Copy link
Collaborator

internally moving the ContentObserver emission to the i/o pool

Since we should initially receive all notifications on the common handler thread (https://github.com/pushtorefresh/storio/blob/master/storio-content-resolver/src/main/java/com/pushtorefresh/storio/contentresolver/impl/RxChangesObserver.java#L40) moving emission to thread from io pool will add delay due to thread switching =(
If user will agree with such delay defaultObserveOnScheduler(Schedulers.io()) will do this trick
If you prefer to manually configure schedulers on every case and get rid of such delay - simply use defaultObserveOnScheduler(null)

asRxObservable(Scheduler)

I'm not sure that asRxObservable(Scheduler) will give more benefits over asRxObservable().observerOn(Scheduler)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants