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

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

Open
3 tasks done
Jaehwa-Noh opened this issue May 9, 2024 · 7 comments · May be fixed by #1238
Open
3 tasks done

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

Jaehwa-Noh opened this issue May 9, 2024 · 7 comments · May be fixed by #1238
Labels
bug Something isn't working

Comments

@Jaehwa-Noh
Copy link
Contributor

Jaehwa-Noh commented May 9, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Is there a StackOverflow question about this issue?

  • I have searched StackOverflow

What happened?

map method in combine flow works on Main thread.


image

image

image

|image

Relevant logcat output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@Jaehwa-Noh Jaehwa-Noh added the bug Something isn't working label May 9, 2024
@Jaehwa-Noh Jaehwa-Noh linked a pull request May 9, 2024 that will close this issue
@yongsuk44
Copy link
Contributor

yongsuk44 commented May 12, 2024

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
Copy link
Contributor Author

Jaehwa-Noh commented May 12, 2024

@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
Copy link
Contributor

yongsuk44 commented May 12, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor

yongsuk44 commented May 12, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants