Skip to content

Commit e2f94e4

Browse files
Fix part of #5312, part of #59: Introduce better script execution support (#5313)
## Explanation Fixes part of #5312 Fixes part of #59 This PR helps prepare for changes coming in #5315 and #4929 (the latter of which is the start of the main upcoming Bazel migration PR chain) by introducing one main scripts-based change: ``ScriptBackgroundCoroutineDispatcher``: a Kotlin coroutine dispatcher for executing asynchronous tasks in scripts that also supports proper Java executor service shutdown (so that scripts don't hang). This dispatcher is multi-threaded to help simplify executing large numbers of parallel background tasks. All scripts have been migrated over to running their primary operations within the context of this new dispatcher. Relevant script utilities have been updated to use it, including ``CommandExecutor`` (though this is mainly a placeholder change for the main executor changes which are coming in #4929). Miscellaneous details to note: 1. A bunch of 'e.g.' typos were fixed in ``GenerateMavenDependenciesList.kt`` and ``wiki/Updating-Maven-Dependencies.md``. These aren't functionally needed, they were just something I noticed while developing. 2. ``kotlinx-coroutines-core`` was updated from 1.4.1 to 1.4.3 in order to work around Kotlin/kotlinx.coroutines#2371 which was causing flakiness in one of the new dispatcher tests. 3. ``testClose_pendingTaskLongerThanCloseTimeout_taskIsNotRun`` intentionally takes ~2 seconds to run in order to provide some assurance that, without cancellation, the task _would_ run and the test _would_ fail (this has been manually verified in a few different situations of the dispatcher and/or test changing; some changes won't result in a failure due to how cancellation works internally for executor service & the converted coroutine dispatcher). Note that historically these changes were originally part of #4929, but they were split out so that they could be used by #5315 (which ended up being convenient to include prior to #4929). ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only This PR doesn't include any user-facing changes since it only impacts scripts. --------- Co-authored-by: Adhiambo Peres <[email protected]>
1 parent 0cc0237 commit e2f94e4

25 files changed

+557
-217
lines changed

scripts/assets/maven_dependencies.textproto

+13-2
Original file line numberDiff line numberDiff line change
@@ -1050,8 +1050,19 @@ maven_dependency {
10501050
}
10511051
}
10521052
maven_dependency {
1053-
artifact_name: "org.jetbrains.kotlinx:kotlinx-coroutines-core:1.4.1"
1054-
artifact_version: "1.4.1"
1053+
artifact_name: "org.jetbrains.kotlinx:kotlinx-coroutines-core-jvm:1.4.3"
1054+
artifact_version: "1.4.3"
1055+
license {
1056+
license_name: "The Apache Software License, Version 2.0"
1057+
original_link: "https://www.apache.org/licenses/LICENSE-2.0.txt"
1058+
scrapable_link {
1059+
url: "https://www.apache.org/licenses/LICENSE-2.0.txt"
1060+
}
1061+
}
1062+
}
1063+
maven_dependency {
1064+
artifact_name: "org.jetbrains.kotlinx:kotlinx-coroutines-core:1.4.3"
1065+
artifact_version: "1.4.3"
10551066
license {
10561067
license_name: "The Apache Software License, Version 2.0"
10571068
original_link: "https://www.apache.org/licenses/LICENSE-2.0.txt"

scripts/src/java/org/oppia/android/scripts/build/TransformAndroidManifest.kt

+24-19
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package org.oppia.android.scripts.build
22

3+
import org.oppia.android.scripts.common.CommandExecutorImpl
34
import org.oppia.android.scripts.common.GitClient
5+
import org.oppia.android.scripts.common.ScriptBackgroundCoroutineDispatcher
46
import org.w3c.dom.Document
57
import org.w3c.dom.NodeList
68
import java.io.File
@@ -62,21 +64,24 @@ fun main(args: Array<String>) {
6264
check(args.size >= 9) { USAGE_STRING }
6365

6466
val repoRoot = File(args[0]).also { if (!it.exists()) error("File doesn't exist: ${args[0]}") }
65-
TransformAndroidManifest(
66-
repoRoot = repoRoot,
67-
sourceManifestFile = File(args[1]).also {
68-
if (!it.exists()) {
69-
error("File doesn't exist: ${args[1]}")
70-
}
71-
},
72-
outputManifestFile = File(args[2]),
73-
buildFlavor = args[3],
74-
majorVersion = args[4].toIntOrNull() ?: error(USAGE_STRING),
75-
minorVersion = args[5].toIntOrNull() ?: error(USAGE_STRING),
76-
versionCode = args[6].toIntOrNull() ?: error(USAGE_STRING),
77-
relativelyQualifiedApplicationClass = args[7],
78-
baseDevelopBranchReference = args[8]
79-
).generateAndOutputNewManifest()
67+
ScriptBackgroundCoroutineDispatcher().use { scriptBgDispatcher ->
68+
TransformAndroidManifest(
69+
repoRoot = repoRoot,
70+
sourceManifestFile = File(args[1]).also {
71+
if (!it.exists()) {
72+
error("File doesn't exist: ${args[1]}")
73+
}
74+
},
75+
outputManifestFile = File(args[2]),
76+
buildFlavor = args[3],
77+
majorVersion = args[4].toIntOrNull() ?: error(USAGE_STRING),
78+
minorVersion = args[5].toIntOrNull() ?: error(USAGE_STRING),
79+
versionCode = args[6].toIntOrNull() ?: error(USAGE_STRING),
80+
relativelyQualifiedApplicationClass = args[7],
81+
baseDevelopBranchReference = args[8],
82+
scriptBgDispatcher
83+
).generateAndOutputNewManifest()
84+
}
8085
}
8186

8287
private class TransformAndroidManifest(
@@ -88,11 +93,11 @@ private class TransformAndroidManifest(
8893
private val minorVersion: Int,
8994
private val versionCode: Int,
9095
private val relativelyQualifiedApplicationClass: String,
91-
private val baseDevelopBranchReference: String
96+
private val baseDevelopBranchReference: String,
97+
private val scriptBgDispatcher: ScriptBackgroundCoroutineDispatcher
9298
) {
93-
private val gitClient by lazy {
94-
GitClient(repoRoot, baseDevelopBranchReference)
95-
}
99+
private val commandExecutor by lazy { CommandExecutorImpl(scriptBgDispatcher) }
100+
private val gitClient by lazy { GitClient(repoRoot, baseDevelopBranchReference, commandExecutor) }
96101
private val documentBuilderFactory by lazy { DocumentBuilderFactory.newInstance() }
97102
private val transformerFactory by lazy { TransformerFactory.newInstance() }
98103

scripts/src/java/org/oppia/android/scripts/ci/ComputeAffectedTests.kt

+11-8
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import org.oppia.android.scripts.common.CommandExecutor
55
import org.oppia.android.scripts.common.CommandExecutorImpl
66
import org.oppia.android.scripts.common.GitClient
77
import org.oppia.android.scripts.common.ProtoStringEncoder.Companion.toCompressedBase64
8+
import org.oppia.android.scripts.common.ScriptBackgroundCoroutineDispatcher
89
import org.oppia.android.scripts.proto.AffectedTestsBucket
910
import java.io.File
1011
import java.util.Locale
@@ -59,9 +60,10 @@ fun main(args: Array<String>) {
5960
" '$computeAllTestsValue'"
6061
)
6162
}
62-
ComputeAffectedTests().compute(
63-
pathToRoot, pathToOutputFile, baseDevelopBranchReference, computeAllTestsSetting
64-
)
63+
ScriptBackgroundCoroutineDispatcher().use { scriptBgDispatcher ->
64+
ComputeAffectedTests(scriptBgDispatcher)
65+
.compute(pathToRoot, pathToOutputFile, baseDevelopBranchReference, computeAllTestsSetting)
66+
}
6567
}
6668

6769
// Needed since the codebase isn't yet using Kotlin 1.5, so this function isn't available.
@@ -75,10 +77,11 @@ private fun String.toBooleanStrictOrNull(): Boolean? {
7577

7678
/** Utility used to compute affected test targets. */
7779
class ComputeAffectedTests(
78-
private val maxTestCountPerLargeShard: Int = MAX_TEST_COUNT_PER_LARGE_SHARD,
79-
private val maxTestCountPerMediumShard: Int = MAX_TEST_COUNT_PER_MEDIUM_SHARD,
80-
private val maxTestCountPerSmallShard: Int = MAX_TEST_COUNT_PER_SMALL_SHARD,
81-
private val commandExecutor: CommandExecutor = CommandExecutorImpl()
80+
private val scriptBgDispatcher: ScriptBackgroundCoroutineDispatcher,
81+
val maxTestCountPerLargeShard: Int = MAX_TEST_COUNT_PER_LARGE_SHARD,
82+
val maxTestCountPerMediumShard: Int = MAX_TEST_COUNT_PER_MEDIUM_SHARD,
83+
val maxTestCountPerSmallShard: Int = MAX_TEST_COUNT_PER_SMALL_SHARD,
84+
val commandExecutor: CommandExecutor = CommandExecutorImpl(scriptBgDispatcher)
8285
) {
8386
private companion object {
8487
private const val GENERIC_TEST_BUCKET_NAME = "generic"
@@ -108,7 +111,7 @@ class ComputeAffectedTests(
108111

109112
println("Running from directory root: $rootDirectory")
110113

111-
val gitClient = GitClient(rootDirectory, baseDevelopBranchReference)
114+
val gitClient = GitClient(rootDirectory, baseDevelopBranchReference, commandExecutor)
112115
val bazelClient = BazelClient(rootDirectory, commandExecutor)
113116
println("Current branch: ${gitClient.currentBranch}")
114117
println("Most recent common commit: ${gitClient.branchMergeBase}")

scripts/src/java/org/oppia/android/scripts/common/BUILD.bazel

+13
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ kt_jvm_library(
3636
"CommandResult.kt",
3737
],
3838
visibility = ["//scripts:oppia_script_library_visibility"],
39+
deps = [
40+
":script_background_coroutine_dispatcher",
41+
"//third_party:org_jetbrains_kotlinx_kotlinx-coroutines-core",
42+
],
3943
)
4044

4145
kt_jvm_library(
@@ -57,3 +61,12 @@ kt_jvm_library(
5761
"//third_party:org_jetbrains_kotlin_kotlin-stdlib-jdk8_jar",
5862
],
5963
)
64+
65+
kt_jvm_library(
66+
name = "script_background_coroutine_dispatcher",
67+
srcs = ["ScriptBackgroundCoroutineDispatcher.kt"],
68+
visibility = ["//scripts:oppia_script_library_visibility"],
69+
deps = [
70+
"//third_party:org_jetbrains_kotlinx_kotlinx-coroutines-core",
71+
],
72+
)

scripts/src/java/org/oppia/android/scripts/common/BazelClient.kt

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import java.util.Locale
1010
*/
1111
class BazelClient(
1212
private val rootDirectory: File,
13-
private val commandExecutor: CommandExecutor = CommandExecutorImpl()
13+
private val commandExecutor: CommandExecutor
1414
) {
1515
/** Returns all Bazel test targets in the workspace. */
1616
fun retrieveAllTestTargets(): List<String> {

scripts/src/java/org/oppia/android/scripts/common/CommandExecutorImpl.kt

+9-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
package org.oppia.android.scripts.common
22

3+
import kotlinx.coroutines.CoroutineScope
4+
import kotlinx.coroutines.async
5+
import kotlinx.coroutines.runBlocking
36
import java.io.File
47
import java.util.concurrent.TimeUnit
58

@@ -11,6 +14,7 @@ const val WAIT_PROCESS_TIMEOUT_MS = 60_000L
1114

1215
/** Default implementation of [CommandExecutor]. */
1316
class CommandExecutorImpl(
17+
private val scriptBgDispatcher: ScriptBackgroundCoroutineDispatcher,
1418
private val processTimeout: Long = WAIT_PROCESS_TIMEOUT_MS,
1519
private val processTimeoutUnit: TimeUnit = TimeUnit.MILLISECONDS
1620
) : CommandExecutor {
@@ -29,7 +33,11 @@ class CommandExecutorImpl(
2933
.directory(workingDir)
3034
.redirectErrorStream(includeErrorOutput)
3135
.start()
32-
val finished = process.waitFor(processTimeout, processTimeoutUnit)
36+
val finished = runBlocking {
37+
CoroutineScope(scriptBgDispatcher).async {
38+
process.waitFor(processTimeout, processTimeoutUnit)
39+
}.await()
40+
}
3341
check(finished) { "Process did not finish within the expected timeout" }
3442
return CommandResult(
3543
process.exitValue(),

scripts/src/java/org/oppia/android/scripts/common/GitClient.kt

+2-3
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,9 @@ import java.io.File
99
*/
1010
class GitClient(
1111
private val workingDirectory: File,
12-
private val baseDevelopBranchReference: String
12+
private val baseDevelopBranchReference: String,
13+
private val commandExecutor: CommandExecutor
1314
) {
14-
private val commandExecutor by lazy { CommandExecutorImpl() }
15-
1615
/** The commit hash of the HEAD of the local Git repository. */
1716
val currentCommit: String by lazy { retrieveCurrentCommit() }
1817

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
package org.oppia.android.scripts.common
2+
3+
import kotlinx.coroutines.CoroutineDispatcher
4+
import kotlinx.coroutines.Runnable
5+
import kotlinx.coroutines.asCoroutineDispatcher
6+
import java.io.Closeable
7+
import java.util.concurrent.ExecutorService
8+
import java.util.concurrent.Executors
9+
import java.util.concurrent.TimeUnit
10+
import kotlin.coroutines.CoroutineContext
11+
12+
/**
13+
* A [CoroutineDispatcher] that's [Closeable] and particularly tailored to be easily used in scripts
14+
* that need to perform parallel tasks for expensive IO. It's highly recommended to exclusively use
15+
* this dispatcher over any others, and to ensure that [close] is called at the end of the script to
16+
* avoid any potential threads hanging (causing the script to not actually close).
17+
*
18+
* Note that the dispatcher attempts to finish any ongoing tasks when [close] is called, but it will
19+
* reject new tasks from being scheduled and it will force terminate if any pending tasks at the
20+
* time of closing don't end within the configured [closeTimeout] provided.
21+
*
22+
* A simple example for using this dispatcher:
23+
* ```kotlin
24+
* ScriptBackgroundCoroutineDispatcher().use { scriptBgDispatcher ->
25+
* val deferred = CoroutineScope(scriptBgDispatcher).async {
26+
* // Expensive task...
27+
* }
28+
* // IMPORTANT: The operation must be observed before use{} ends, otherwise the dispatcher will
29+
* // close and terminate any pending tasks.
30+
* runBlocking { deferred.await() }
31+
* }
32+
* ```
33+
*
34+
* A more complex example for I/O operations:
35+
* ```kotlin
36+
* ScriptBackgroundCoroutineDispatcher().use { scriptBgDispatcher ->
37+
* val deferred = CoroutineScope(scriptBgDispatcher).async {
38+
* withContext(Dispatchers.IO) {
39+
* // Perform I/O using Kotlin's highly parallelized I/O dispatcher, but wait for the result
40+
* // using the background script dispatcher (since execution could continue if other I/O
41+
* // operations need to be kicked off, or if other work can be done alongside the I/O).
42+
* }
43+
* }
44+
* // IMPORTANT: The operation must be observed before use{} ends, otherwise the dispatcher will
45+
* // close and terminate any pending tasks.
46+
* runBlocking { deferred.await() }
47+
* }
48+
* ```
49+
*
50+
* @property closeTimeout the amount of time, in [closeTimeoutUnit] units, that should be waited
51+
* when [close]ing this dispatcher before force-ending ongoing tasks
52+
* @property closeTimeoutUnit the unit of time used for [closeTimeout]
53+
*/
54+
class ScriptBackgroundCoroutineDispatcher(
55+
private val closeTimeout: Long = 5,
56+
private val closeTimeoutUnit: TimeUnit = TimeUnit.SECONDS
57+
) : CoroutineDispatcher(), Closeable {
58+
private val threadPool by lazy { Executors.newCachedThreadPool() }
59+
private val coroutineDispatcher by lazy { threadPool.asCoroutineDispatcher() }
60+
61+
override fun dispatch(context: CoroutineContext, block: Runnable) {
62+
coroutineDispatcher.dispatch(context, block)
63+
}
64+
65+
override fun close() {
66+
threadPool.tryShutdownFully(timeout = closeTimeout, unit = closeTimeoutUnit)
67+
coroutineDispatcher.close()
68+
}
69+
70+
private companion object {
71+
private fun ExecutorService.tryShutdownFully(timeout: Long, unit: TimeUnit) {
72+
// Try to fully shutdown the executor service per https://stackoverflow.com/a/33690603 and
73+
// https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ExecutorService.html.
74+
shutdown()
75+
try {
76+
if (!awaitTermination(timeout, unit)) {
77+
shutdownNow()
78+
check(awaitTermination(timeout, unit)) { "ExecutorService didn't fully shutdown: $this." }
79+
}
80+
} catch (e: InterruptedException) {
81+
shutdownNow()
82+
Thread.currentThread().interrupt()
83+
}
84+
}
85+
}
86+
}

scripts/src/java/org/oppia/android/scripts/license/MavenDependenciesListCheck.kt

+6-2
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package org.oppia.android.scripts.license
33
import com.google.protobuf.TextFormat
44
import org.oppia.android.scripts.common.CommandExecutor
55
import org.oppia.android.scripts.common.CommandExecutorImpl
6+
import org.oppia.android.scripts.common.ScriptBackgroundCoroutineDispatcher
67
import org.oppia.android.scripts.proto.MavenDependency
78

89
/**
@@ -24,7 +25,9 @@ import org.oppia.android.scripts.proto.MavenDependency
2425
* third_party/maven_install.json scripts/assets/maven_dependencies.pb
2526
*/
2627
fun main(args: Array<String>) {
27-
MavenDependenciesListCheck(LicenseFetcherImpl()).main(args)
28+
ScriptBackgroundCoroutineDispatcher().use { scriptBgDispatcher ->
29+
MavenDependenciesListCheck(LicenseFetcherImpl(), scriptBgDispatcher).main(args)
30+
}
2831
}
2932

3033
/**
@@ -33,7 +36,8 @@ fun main(args: Array<String>) {
3336
*/
3437
class MavenDependenciesListCheck(
3538
private val licenseFetcher: LicenseFetcher,
36-
private val commandExecutor: CommandExecutor = CommandExecutorImpl()
39+
scriptBgDispatcher: ScriptBackgroundCoroutineDispatcher,
40+
private val commandExecutor: CommandExecutor = CommandExecutorImpl(scriptBgDispatcher)
3741
) {
3842

3943
/**

scripts/src/java/org/oppia/android/scripts/license/MavenDependenciesRetriever.kt

+1-2
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import com.squareup.moshi.Moshi
55
import com.squareup.moshi.kotlin.reflect.KotlinJsonAdapterFactory
66
import org.oppia.android.scripts.common.BazelClient
77
import org.oppia.android.scripts.common.CommandExecutor
8-
import org.oppia.android.scripts.common.CommandExecutorImpl
98
import org.oppia.android.scripts.maven.model.MavenListDependency
109
import org.oppia.android.scripts.maven.model.MavenListDependencyTree
1110
import org.oppia.android.scripts.proto.License
@@ -24,7 +23,7 @@ private const val MAVEN_PREFIX = "@maven//:"
2423
class MavenDependenciesRetriever(
2524
private val rootPath: String,
2625
private val licenseFetcher: LicenseFetcher,
27-
private val commandExecutor: CommandExecutor = CommandExecutorImpl()
26+
private val commandExecutor: CommandExecutor
2827
) {
2928

3029
private val bazelClient by lazy {

0 commit comments

Comments
 (0)