-
Notifications
You must be signed in to change notification settings - Fork 1.3k
15278 detekt rule runblocking #15942
Changes from 13 commits
266786b
da7b1ed
1214bb6
3fd69e2
d526980
d1db99f
5f1c747
c277469
de6a96d
745e9af
da0f4c6
bb5636d
a121a9b
8d63ebc
36fc3e1
676fb6b
5f2da80
b265a37
e0be45d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,13 +9,15 @@ import androidx.test.uiautomator.UiDevice | |
import org.junit.Assert.assertEquals | ||
import org.junit.Rule | ||
import org.junit.Test | ||
import org.mozilla.fenix.perf.RunBlockingCounter | ||
import org.mozilla.fenix.ext.components | ||
import org.mozilla.fenix.helpers.HomeActivityTestRule | ||
|
||
// PLEASE CONSULT WITH PERF TEAM BEFORE CHANGING THIS VALUE. | ||
private const val EXPECTED_SUPPRESSION_COUNT = 11 | ||
|
||
private const val FAILURE_MSG = """StrictMode startup suppression count does not match expected count. | ||
private const val EXPECTED_RUNBLOCKING_COUNT = 2 | ||
private const val STRICTMODE_FAILURE_MSG = """StrictMode startup suppression count does not match expected count. | ||
|
||
If this PR removed code that suppressed StrictMode, great! Please decrement the suppression count. | ||
|
||
|
@@ -28,6 +30,19 @@ private const val FAILURE_MSG = """StrictMode startup suppression count does not | |
|
||
""" | ||
|
||
private const val RUNBLOCKING_FAILURE_MSG = """RunblockingCounter startup count does not match expected count. | ||
|
||
If this PR removed code that removed a RunBlocking call, great! Please decrement the suppression count. | ||
|
||
Did this PR add or call code that incremented the RunBlockingCounter? | ||
Did you know that using runBlocking can have negative perfomance implications? | ||
|
||
If so, please do your best to implement a solution without blocking the main thread. | ||
Please consult the perf team if you have questions or believe that using runBlocking | ||
is the optimal solution. | ||
|
||
""" | ||
|
||
/** | ||
* A performance test to limit the number of StrictMode suppressions on startup. | ||
* This test was written by the perf team. | ||
|
@@ -39,20 +54,23 @@ private const val FAILURE_MSG = """StrictMode startup suppression count does not | |
* | ||
* IF YOU UPDATE THE TEST NAME, UPDATE CODE OWNERS. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you read the comment? 😝 Can you add this to code owners? |
||
*/ | ||
class StrictModeStartupSuppressionCountTest { | ||
class StartupExcessiveResourceUseTest { | ||
@get:Rule | ||
val activityTestRule = HomeActivityTestRule(skipOnboarding = true) | ||
|
||
private val uiDevice = UiDevice.getInstance(InstrumentationRegistry.getInstrumentation()) | ||
|
||
@Test | ||
fun verifyStrictModeSuppressionCount() { | ||
fun verifyRunBlockingAndStrictModeSuppresionCount() { | ||
uiDevice.waitForIdle() // wait for async UI to load. | ||
|
||
// This might cause intermittents: at an arbitrary point after start up (such as the visual | ||
// completeness queue), we might run code on the main thread that suppresses StrictMode, | ||
// causing this number to fluctuate depending on device speed. We'll deal with it if it occurs. | ||
val actual = activityTestRule.activity.components.strictMode.suppressionCount.toInt() | ||
assertEquals(FAILURE_MSG, EXPECTED_SUPPRESSION_COUNT, actual) | ||
val actualSuppresionCount = activityTestRule.activity.components.strictMode.suppressionCount.toInt() | ||
val actualRunBlocking = RunBlockingCounter.count | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an |
||
|
||
assertEquals(STRICTMODE_FAILURE_MSG, EXPECTED_SUPPRESSION_COUNT, actualSuppresionCount) | ||
assertEquals(RUNBLOCKING_FAILURE_MSG, EXPECTED_RUNBLOCKING_COUNT, actualRunBlocking) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,6 @@ import kotlinx.coroutines.Dispatchers | |
import kotlinx.coroutines.Job | ||
import kotlinx.coroutines.async | ||
import kotlinx.coroutines.launch | ||
import kotlinx.coroutines.runBlocking | ||
import mozilla.components.browser.search.SearchEngine | ||
import mozilla.components.browser.search.provider.AssetsSearchEngineProvider | ||
import mozilla.components.browser.search.provider.SearchEngineList | ||
|
@@ -27,6 +26,7 @@ import org.mozilla.fenix.BuildConfig | |
import org.mozilla.fenix.Config | ||
import org.mozilla.fenix.ext.components | ||
import org.mozilla.fenix.ext.settings | ||
import org.mozilla.fenix.perf.runBlockingIncrement | ||
import java.util.Locale | ||
|
||
@SuppressWarnings("TooManyFunctions") | ||
|
@@ -116,40 +116,47 @@ open class FenixSearchEngineProvider( | |
* @return a list of all SearchEngines that are currently active. These are the engines that | ||
* are readily available throughout the app. | ||
*/ | ||
fun installedSearchEngines(context: Context): SearchEngineList = runBlocking { | ||
val installedIdentifiers = installedSearchEngineIdentifiers(context) | ||
val engineList = searchEngines.await() | ||
|
||
engineList.copy( | ||
list = engineList.list.filter { | ||
installedIdentifiers.contains(it.identifier) | ||
}.sortedBy { it.name.toLowerCase(Locale.getDefault()) }, | ||
default = engineList.default?.let { | ||
if (installedIdentifiers.contains(it.identifier)) { | ||
it | ||
} else { | ||
null | ||
fun installedSearchEngines(context: Context): SearchEngineList = | ||
runBlockingIncrement { | ||
val installedIdentifiers = installedSearchEngineIdentifiers(context) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we avoid breaking blame? Or is it impossible because the line is too long? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Blame is still broken here: it's double-indented. I think it'd be fixed if we corrected the indentation. |
||
val engineList = searchEngines.await() | ||
|
||
engineList.copy( | ||
list = engineList.list.filter { | ||
installedIdentifiers.contains(it.identifier) | ||
}.sortedBy { it.name.toLowerCase(Locale.getDefault()) }, | ||
default = engineList.default?.let { | ||
if (installedIdentifiers.contains(it.identifier)) { | ||
it | ||
} else { | ||
null | ||
} | ||
} | ||
} | ||
) | ||
} | ||
) | ||
} | ||
|
||
fun allSearchEngineIdentifiers() = runBlocking { | ||
loadedSearchEngines.await().list.map { it.identifier } | ||
} | ||
fun allSearchEngineIdentifiers() = | ||
runBlockingIncrement { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: avoid blame |
||
loadedSearchEngines.await().list.map { it.identifier } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: same indentation issues here and below. |
||
} | ||
|
||
fun uninstalledSearchEngines(context: Context): SearchEngineList = runBlocking { | ||
val installedIdentifiers = installedSearchEngineIdentifiers(context) | ||
val engineList = loadedSearchEngines.await() | ||
fun uninstalledSearchEngines(context: Context): SearchEngineList = | ||
runBlockingIncrement { | ||
val installedIdentifiers = installedSearchEngineIdentifiers(context) | ||
val engineList = loadedSearchEngines.await() | ||
|
||
engineList.copy(list = engineList.list.filterNot { installedIdentifiers.contains(it.identifier) }) | ||
} | ||
engineList.copy(list = engineList.list.filterNot { installedIdentifiers.contains(it.identifier) }) | ||
} | ||
|
||
override suspend fun loadSearchEngines(context: Context): SearchEngineList { | ||
return installedSearchEngines(context) | ||
} | ||
|
||
fun installSearchEngine(context: Context, searchEngine: SearchEngine, isCustom: Boolean = false) = runBlocking { | ||
fun installSearchEngine( | ||
context: Context, | ||
searchEngine: SearchEngine, | ||
isCustom: Boolean = false | ||
) = runBlockingIncrement { | ||
if (isCustom) { | ||
val searchUrl = searchEngine.getSearchTemplate() | ||
CustomSearchEngineStore.addSearchEngine(context, searchEngine.name, searchUrl) | ||
|
@@ -162,14 +169,19 @@ open class FenixSearchEngineProvider( | |
} | ||
} | ||
|
||
fun uninstallSearchEngine(context: Context, searchEngine: SearchEngine, isCustom: Boolean = false) = runBlocking { | ||
fun uninstallSearchEngine( | ||
context: Context, | ||
searchEngine: SearchEngine, | ||
isCustom: Boolean = false | ||
) = runBlockingIncrement { | ||
if (isCustom) { | ||
CustomSearchEngineStore.removeSearchEngine(context, searchEngine.identifier) | ||
reload() | ||
} else { | ||
val installedIdentifiers = installedSearchEngineIdentifiers(context).toMutableSet() | ||
installedIdentifiers.remove(searchEngine.identifier) | ||
prefs(context).edit().putStringSet(localeAwareInstalledEnginesKey(), installedIdentifiers).apply() | ||
prefs(context).edit() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: you didn't change this line so don't break blame |
||
.putStringSet(localeAwareInstalledEnginesKey(), installedIdentifiers).apply() | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,6 @@ import android.content.Intent | |
import android.content.Intent.FLAG_ACTIVITY_NEW_DOCUMENT | ||
import androidx.annotation.VisibleForTesting | ||
import androidx.core.content.ContextCompat | ||
import kotlinx.coroutines.runBlocking | ||
import mozilla.components.browser.session.Session | ||
import mozilla.components.browser.session.SessionManager | ||
import mozilla.components.browser.state.state.CustomTabConfig | ||
|
@@ -28,6 +27,7 @@ import mozilla.components.support.utils.SafeIntent | |
import mozilla.components.support.utils.toSafeIntent | ||
import org.json.JSONObject | ||
import org.mozilla.fenix.R | ||
import org.mozilla.fenix.perf.runBlockingIncrement | ||
import java.io.File | ||
import java.io.IOException | ||
|
||
|
@@ -58,7 +58,12 @@ class FennecWebAppIntentProcessor( | |
val url = safeIntent.dataString | ||
|
||
return if (!url.isNullOrEmpty() && matches(intent)) { | ||
val webAppManifest = runBlocking { loadManifest(safeIntent, url) } | ||
val webAppManifest = runBlockingIncrement { | ||
loadManifest( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: one line for |
||
safeIntent, | ||
url | ||
) | ||
} | ||
|
||
val session = Session(url, private = false, source = SessionState.Source.HOME_SCREEN) | ||
session.webAppManifest = webAppManifest | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
/* This Source Code Form is subject to the terms of the Mozilla Public | ||
* License, v. 2.0. If a copy of the MPL was not distributed with this | ||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
||
// This class implements the alternative ways to invoke runBlocking with some | ||
// monitoring by wrapping the raw methods. This lint check tells us not to use the raw | ||
// methods so we suppress the check. | ||
@file:Suppress("MozillaRunBlockingCheck") | ||
|
||
package org.mozilla.fenix.perf | ||
|
||
import androidx.annotation.VisibleForTesting | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.runBlocking | ||
import kotlin.coroutines.CoroutineContext | ||
|
||
/** | ||
* Counts the number of runBlocking calls made | ||
*/ | ||
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) | ||
object RunBlockingCounter { | ||
var count = 0L | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle overflow? fwiw, I like handling it here, in the setter, rather than in runBlockingIncrement because it keeps the other method simple. That being said, it might not be possible with |
||
@Synchronized set | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per my earlier comment, this might just need to be volatile – can you check how java assignment works? Is |
||
} | ||
|
||
/** | ||
* Wrapper around `runBlocking`. RunBlocking seems to be a "fix-all" to return values to the thread | ||
* where the coroutine is called. The official doc explains runBlocking: "Runs a new coroutine and | ||
* blocks the current thread interruptibly until its completion`. This can have negative | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: "negative side-effects on our main thread"? Maybe we should just say, "block the main thread" |
||
* side-effects on the our main thread which could lead to significant jank. This wrapper aims to | ||
* count the number of runBlocking call to try to limit them as much as possible to encourage | ||
* alternatives solutions whenever this function might be needed. | ||
*/ | ||
fun <T> runBlockingIncrement( | ||
context: CoroutineContext? = null, | ||
action: suspend CoroutineScope.() -> T | ||
): T { | ||
RunBlockingCounter.count += 1 | ||
|
||
context?.let { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't do what you think it does. It does: if (context != null) {
val returnValue = runBlocking(context) { action() }
if (returnValue != null) {
runBlocking(action())
}
} I guess unless return return from the containing function and not the lambda. Even if the code is correct, the fact that it can be misinterpreted isn't great. In general, it's good to be careful when using symbols ( I would prefer return if (context != null) {
runBlocking(it, ::action)
} else {
runBlocking(::action)
} Note that the function reference is an optimization that avoids us having to create a new lambda (if the compiler isn't smart enough to do it for us). Note also the deduplication of the |
||
return runBlocking(it) { action() } | ||
} ?: run { | ||
return runBlocking { action() } | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: update the comment