Wordpress-android: Remove usages of Coroutine's GlobalScope in Stores

[malinajirka]

GlobalScope.launch(..) is used in some of the Stores (eg. here). According to the documentation
Application code usually should use application-defined CoroutineScope, using async or launch on the instance of GlobalScope is highly discouraged.

I'm still not sure what's the best approach so we'll need to explore the options first.

[oguzkocer]

cc @shiki @0nko @planarvoid

[0nko]

I agree with that statement. The GlobalScope has been because we started using coroutines before the scopes were introduced. For every coroutine we use, we should think what its lifecycle should be. If it's bound by the screen, it should be the VM scope, for longer we should to create an application-bound scope.

[stale[bot]]

This issue has been marked as stale because:

• It has been inactive for the past year.
• It isn't in a project or a milestone.
• It hasn’t been labeled [Pri] Blocker, [Pri] High, or good first issue.

Please comment with an update if you believe this issue is still valid or if it can be closed. This issue will also be reviewed for validity and priority (cc @designsimply).

[planarvoid]

There is one usecase I'm wondering about with the GlobalScope and that's when you want to do a fire&forget kind of operation that you don't want to tie to the activity lifecycle. Do you think it would make sense to introduce something like ApplicationScope? I use GlobalScope in cases like that (and I believe it is still valid) but maybe there is a better way - @malinajirka @0nko

[0nko]

I haven't had a chance to think about this in a while. I think GlobalScope is still valid for certain cases that really span the app lifecycle. But I think we are missing something in between this and the viewModelScope.

Say you want to perform some action, leave the current screen and then show the result in a snackbar on after the action completes. You can't use the viewModelScope so you're stuck with GlobalScope. But that's an overkill and if the action get stuck you'll leak a coroutine. A way to solve this would be if you could supply a coroutine scope from the calling ViewModel to one being called.

[planarvoid]

@0nko I agree doing this is gonna be a bit tricky. I think though that the GlobalScope is tied to the Application life cycle and the only thing that should cancel the call is when the Application is killed (when we want the call happen no matter what). One thing that would be better is to not use coroutines for this use case and have a system in place that would somehow put the action we want to perform globally into a queue which would be processed later by a sync service. For example saving a post when leaving the editor is something we don't want to tie to the ViewModel lifecycle but I think it's perfectly fine for it to happen a bit later (when the system is ready).

GlobalScope actually feels like a workaround for a case like this.

[malinajirka]

There is one usecase I'm wondering about with the GlobalScope and that's when you want to do a fire&forget kind of operation that you don't want to tie to the activity lifecycle. Do you think it would make sense to introduce something like ApplicationScope? I use GlobalScope in cases like that (and I believe it is still valid) but maybe there is a better way -

I agree, that GlobalScope is valid for such usecases. I'm not sure if it's necessary to introduce an Application scope as GlobalScope is technically an Application scope, right?

Say you want to perform some action, leave the current screen and then show the result in a snackbar on after the action completes. You can't use the viewModelScope so you're stuck with GlobalScope. But that's an overkill and if the action get stuck you'll leak a coroutine. A way to solve this would be if you could supply a coroutine scope from the calling ViewModel to one being called.

It really depends on the usage and architecture of your activities/fragments. If the screens share the same activity, you can still tie VM's lifecycle to the activity and share the scope between the fragments. I don't think there is anything else we could tie it to but Fragment/Activity/Application/Service, right?

As for the SnackBar scenario, if you want to show the snackbar no-matter the screen, I think it's a valid use-case for an EventBus. Wdyt?

For example saving a post when leaving the editor is something we don't want to tie to the ViewModel lifecycle but I think it's perfectly fine for it to happen a bit later (when the system is ready).

Yeah, I think saving a post would ideally be handled by a Service. Service can be easily automatically restored when it's killed. On the other hand saving something into db should be very quick and using a Service for such case seems like an overkill. Moreover, we'd need to send the data in a bundle and we might exceed the max size. I think using GlobalScope with withTimeout is good enough for these types of scenarios. Wdyt?

[planarvoid]

Thanks for the reply @malinajirka 👍

I agree, that GlobalScope is valid for such usecases. I'm not sure if it's necessary to introduce an Application scope as GlobalScope is technically an Application scope, right?

I've actually looked into it more and I think it makes sense to have something like a Store scope. This would basically mean Application scope (until the Application kills the store). This gives us more flexibility for possible future improvements (have a Store scope?).

As for the SnackBar scenario, if you want to show the snackbar no-matter the screen, I think it's a valid use-case for an EventBus. Wdyt?

I agree until we introduce a SnackBar reactive repository :-)

Yeah, I think saving a post would ideally be handled by a Service. Service can be easily automatically restored when it's killed. On the other hand saving something into db should be very quick and using a Service for such case seems like an overkill. Moreover, we'd need to send the data in a bundle and we might exceed the max size. I think using GlobalScope with withTimeout is good enough for these types of scenarios. Wdyt?

That makes sense. Let's see what you think about my solution. I can still reintroduce global scope if it doesn't make sense.