duckduckgo / Android

DuckDuckGo Android App
https://play.google.com/store/apps/details?id=com.duckduckgo.mobile.android
Apache License 2.0
3.77k stars 888 forks source link

[Bug] runBlocking calls from inside coroutine #4409

Closed bbrockbernd closed 5 months ago

bbrockbernd commented 5 months ago

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: https://github.com/duckduckgo/Android/blob/dd6fbf0df4911ba085d2c37c05f861fb1e2958f3/network-protection/network-protection-impl/src/main/java/com/duckduckgo/networkprotection/impl/waitlist/NetworkProtectionWaitlistImpl.kt#L110-L112

Is called from isTreated, which is a suspend function: https://github.com/duckduckgo/Android/blob/dd6fbf0df4911ba085d2c37c05f861fb1e2958f3/network-protection/network-protection-impl/src/main/java/com/duckduckgo/networkprotection/impl/waitlist/NetworkProtectionWaitlistImpl.kt#L94-L101

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. https://github.com/duckduckgo/Android/blob/4d756eb80aa88e334db91951156783a0b0562383/app-tracking-protection/vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/apps/TrackingProtectionAppsRepository.kt#L91-L106

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:
github-actions[bot] commented 5 months ago

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 commented 5 months ago

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