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] runBlocking calls from inside coroutine #4409

Closed
bbrockbernd opened this issue Apr 9, 2024 · 2 comments
Closed

[Bug] runBlocking calls from inside coroutine #4409

bbrockbernd opened this issue Apr 9, 2024 · 2 comments

Comments

@bbrockbernd
Copy link

Describe the bug

There are two instances where a runBlocking builder is called from within a coroutine. Although there are rare occasions where blocking a coroutine is desired, in general this should be avoided. Since it can hurt performance and in bad cases result in deadlocks.

Instance 1
Function didJoinWaitList containing a runBlocking call:

private fun didJoinWaitlist(): Boolean {
return runBlocking { netPWaitlistRepository.getWaitlistToken() != null }
}

Is called from isTreated, which is a suspend function:

private suspend fun isTreated(): Boolean {
if (appBuildConfig.isInternalBuild()) {
// internal users are always treated
return true
}
// User is in beta already
if (didJoinBeta() || didJoinWaitlist()) {
return netPRemoteFeature.isWaitlistActive()

Fix 1
The didJoinWaitList can be turned into a suspend function since it is only called from other suspend functions, which in turn allows the runBlocking builder to be removed.

Instance 2
runBlocking call from within a flow operation (Flow.combine). The passed lambda is a suspend function.

.combine(appTrackerRepository.getManualAppExclusionListFlow()) { ddgExclusionList, manualList ->
logcat { "getProtectedApps flow" }
installedApps
.map {
val isExcluded = isExcludedFromAppTP(it, ddgExclusionList, manualList)
runBlocking {
TrackingProtectionAppInfo(
packageName = it.packageName,
name = packageManager.getApplicationLabel(it).toString(),
category = it.parseAppCategory(),
isExcluded = networkProtectionExclusionList.isExcluded(it.packageName) || isExcluded,
knownProblem = hasKnownIssue(it, ddgExclusionList),
userModified = isUserModified(it.packageName, manualList),
)
}
}

Fix 2
InstalledApps can be handled as a flow with Sequence.asFlow() allowing runBlocking to be removed.

How to Reproduce

Goto:
https://github.com/duckduckgo/Android/blob/dd6fbf0df4911ba085d2c37c05f861fb1e2958f3/network-protection/network-protection-impl/src/main/java/com/duckduckgo/networkprotection/impl/waitlist/NetworkProtectionWaitlistImpl.kt
and
https://github.com/duckduckgo/Android/blob/4d756eb80aa88e334db91951156783a0b0562383/app-tracking-protection/vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/apps/TrackingProtectionAppsRepository.kt

Expected behavior

Unblocked coroutines ;)

Environment

- DDG App Version:
- Device:
- OS:
Copy link

github-actions bot commented Apr 9, 2024

Thank you for opening an Issue in our Repository.
The issue has been forwarded to the team and we'll follow up as soon as we have time to investigate.
As stated in our Contribution Guidelines, requests for feedback should be addressed via the Feedback section in the Android app.

@marcosholgado
Copy link
Contributor

Thanks, we are actually removing some of this code soon and will be cleaning this out.

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

2 participants