Fix runBlocking in coroutines

[bbrockbernd]

Summary

I found a few occasions where the runBlocking coroutine builder is called from within other coroutines. This can affect performance since it blocks the thread that is shared among coroutines, and can in some cases lead to the infamous nested runBlocking deadlock. Often an easy fix is to turn the containing function into a suspend function.

Let me know if I missed something! Cheers

Screenshots

Link to pull request in Documentation repository

Any other notes

[home-assistant[bot]]

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

[jpelgrom]

can in some cases lead to the infamous nested runBlocking deadlock. Often an easy fix is to turn the containing function into a suspend function.

Forgive me for asking what could be an obvious question, but can you link to an example or more specific documentation about this? I understand avoiding the use of runBlocking in suspending functions if possible, but some of these changes (especially in ThemesManager) look like moving around runBlocking { ... } for little to no reason to me at first glance.

[bbrockbernd]

Definitely! The documentation states that blocking a coroutine "potentially leads to thread starvation issues". (last sentence of https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/run-blocking.html). Some intuition of why this can be dangerous: https://betterprogramming.pub/how-i-fell-in-kotlins-runblocking-deadlock-trap-and-how-you-can-avoid-it-db9e7c4909f1. In the end of the day, you want to avoid calling runBlocking from coroutines as much as possible.

For instance, the LanguagesManager.saveLang(lang: String?) function calls runBlocking https://github.com/home-assistant/android/blob/177a040f57f92c245773dc521643dd024b701e86/app/src/main/java/io/homeassistant/companion/android/settings/language/LanguagesManager.kt#L41-L42 However this function is called from SettingsPresenterImpl.putString(key: String, value: String?) from within a coroutine launched on the Main dispatcher. And thus blocking the UI thread. https://github.com/home-assistant/android/blob/177a040f57f92c245773dc521643dd024b701e86/app/src/main/java/io/homeassistant/companion/android/settings/SettingsPresenterImpl.kt#L101-L105 Since this is the only call site for saveLang we can easily turn this function into a suspend function and get rid of the runBlocking builder.

Now in the case of ThemesManager the getCurrentTheme() function which contains a runBlocking builder, is called from SettingsPresenterImpl.getString which contains a runBlocking as well. The issue here is that the getCurrentTheme() function is also called from synchronous code. To solve this, you can turn this function into a suspend function and move the runBlocking to the synchronous call sites (in this case ThemesManager.setThemeForWebView and ChangeLog.showChangeLog)

[jpelgrom]

Thanks for the specific example and explanation, that makes it easier to follow :) These changes look good to me.

There are also other places where runBlocking is used - have you checked all or are these just a few that you immediately noticed? I think almost all others are called from normal Android functions or other places where it is not nested in a suspending function, but would appreciate a second opinion.

[bbrockbernd]

No problem! As a matter of fact we are developing a tool that should be able to detect these runBlockings and are currently testing it on open source projects! It found actually one more occurence:

io.homeassistant.companion.android.common.data.servers.ServerManagerImpl.removeServer io.homeassistant.companion.android.common.data.servers.ServerManagerImpl.integrationRepository io.homeassistant.companion.android.common.data.servers.ServerManagerImpl.activeServerId --> runBlocking

However, I did not see a quick solution to solve this one.. The longer the callstack between runBlockings gets the trickier.

[jpelgrom]

Yes that one will be tricky so let's not touch it for now and do this PR first.