android / nowinandroid

A fully functional Android app built entirely with Kotlin and Jetpack Compose
Apache License 2.0
16.62k stars 3.01k forks source link

[Bug]: Some `combine` works on Main thread #1420

Open Jaehwa-Noh opened 4 months ago

Jaehwa-Noh commented 4 months ago

Is there an existing issue for this?

Is there a StackOverflow question about this issue?

What happened?

map method in combine flow works on Main thread.


image

image

image

|image

Relevant logcat output

No response

Code of Conduct

yongsuk44 commented 4 months ago

Personally, umm... I'm not sure why flowOn needs to be added in the Repository and UseCase. I think that specifying a dispatcher in the coroutine builder would make thread management and tracking easier.

If thread switching is implemented in the Repository and UseCase as you've modified, it seems that it could unnecessarily increase the amount of code to manage and make things more complicated.

Jaehwa-Noh commented 4 months ago

@yongsuk44 I added this flowOn on the Repository and UseCase because that comebine using almost all map method. Map would take long time when the collection become bigger, so that the logic need to be run on Dispatchers.Default.

yongsuk44 commented 4 months ago

I think that specifying a dispatcher in the coroutine builder would make thread management and tracking easier.

@Jaehwa-Noh 😀 This comment was left because I think that handling thread management and tracking with flowOn at the start of the coroutine builder or logic would be efficient.

// Starting point of logic
.flowOn(Dispatchers)
.stateIn(...)

// Coroutine builder
viewModelScope.launch(Dispatcher) { ... }
viewModelScope.async(Dispatcher) { ... }

By doing this, wouldn't it be possible to remove the Dispatcher dependencies in the UseCase and repository?

Additionally, I also believe that performing complex computations on Dispatchers.Default is efficient. However, I'm not quite sure if it's efficient to switch the current tasks to Default. It would be effective if the project data size were very large, but in the current project, I'm unsure how much of a performance improvement there would be. Having a benchmark to compare before and after would make it easier to understand. 🤔

Jaehwa-Noh commented 4 months ago

@yongsuk44 I don't think that repository or usecase own coroutine dispatcher would be controlled in viewModel because usecase and repository could be re-used anywhere. Imagine that controlled by coroutineScope in the viewModel. When use that repository or usecase in another viewModel, developer must always consider that viewModelScope is working properly or not, and that leads them to be making mistake easily.

Current data size isn't enormous, but there's no reason we carry out the risk. I don't say this is for improve the performance, I do this for prevent ANR, long calculate on main thread easily bring that error.

yongsuk44 commented 4 months ago

There is no need to worry about ANR as long as the thread is not blocked when working on Dispatchers.Main. Most functions in our current project are designed as suspend functions, so they should be fine.

Of course, if there are incredibly complex calculations or tasks that block the thread, it would be reasonable to switch to Dispatchers.Default or Dispatchers.IO. However, these operations often involve calls to Room or Retrofit, most of which support the suspend modifier, so I think they should be fine.

As I mentioned in a previous comment, I'm not sure if the increased amount of code that needs to be managed by adding flowOn is worth it.

Am I missing anything here? 🤔

Jaehwa-Noh commented 4 months ago

@yongsuk44 You can check this documentation.

Dispatchers.Main - Use this dispatcher to run a coroutine on the main Android thread. This should be used only for interacting with the UI and performing quick work. Examples include calling suspend functions, running Android UI framework operations, and updating LiveData objects.

Dispatchers.Main is not safe for ANR. That is why Room and Retrofit using Dispatchers.IO.

yongsuk44 commented 4 months ago

ANR occurs when the main thread is 'blocked' for a certain period of time. The suspension of coroutines and the blocking of threads are distinct concepts.

Additionally, when implementing Room and Retrofit with the suspend modifier, the operations are handled on the IO.