-
Notifications
You must be signed in to change notification settings - Fork 528
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Browse the repository at this point in the history
…port (#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 <59600948+adhiamboperes@users.noreply.github.com>
- Loading branch information
1 parent
0cc0237
commit e2f94e4
Showing
25 changed files
with
557 additions
and
217 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
86 changes: 86 additions & 0 deletions
86
scripts/src/java/org/oppia/android/scripts/common/ScriptBackgroundCoroutineDispatcher.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| package org.oppia.android.scripts.common | ||
|
|
||
| import kotlinx.coroutines.CoroutineDispatcher | ||
| import kotlinx.coroutines.Runnable | ||
| import kotlinx.coroutines.asCoroutineDispatcher | ||
| import java.io.Closeable | ||
| import java.util.concurrent.ExecutorService | ||
| import java.util.concurrent.Executors | ||
| import java.util.concurrent.TimeUnit | ||
| import kotlin.coroutines.CoroutineContext | ||
|
|
||
| /** | ||
| * A [CoroutineDispatcher] that's [Closeable] and particularly tailored to be easily used in scripts | ||
| * that need to perform parallel tasks for expensive IO. It's highly recommended to exclusively use | ||
| * this dispatcher over any others, and to ensure that [close] is called at the end of the script to | ||
| * avoid any potential threads hanging (causing the script to not actually close). | ||
| * | ||
| * Note that the dispatcher attempts to finish any ongoing tasks when [close] is called, but it will | ||
| * reject new tasks from being scheduled and it will force terminate if any pending tasks at the | ||
| * time of closing don't end within the configured [closeTimeout] provided. | ||
| * | ||
| * A simple example for using this dispatcher: | ||
| * ```kotlin | ||
| * ScriptBackgroundCoroutineDispatcher().use { scriptBgDispatcher -> | ||
| * val deferred = CoroutineScope(scriptBgDispatcher).async { | ||
| * // Expensive task... | ||
| * } | ||
| * // IMPORTANT: The operation must be observed before use{} ends, otherwise the dispatcher will | ||
| * // close and terminate any pending tasks. | ||
| * runBlocking { deferred.await() } | ||
| * } | ||
| * ``` | ||
| * | ||
| * A more complex example for I/O operations: | ||
| * ```kotlin | ||
| * ScriptBackgroundCoroutineDispatcher().use { scriptBgDispatcher -> | ||
| * val deferred = CoroutineScope(scriptBgDispatcher).async { | ||
| * withContext(Dispatchers.IO) { | ||
| * // Perform I/O using Kotlin's highly parallelized I/O dispatcher, but wait for the result | ||
| * // using the background script dispatcher (since execution could continue if other I/O | ||
| * // operations need to be kicked off, or if other work can be done alongside the I/O). | ||
| * } | ||
| * } | ||
| * // IMPORTANT: The operation must be observed before use{} ends, otherwise the dispatcher will | ||
| * // close and terminate any pending tasks. | ||
| * runBlocking { deferred.await() } | ||
| * } | ||
| * ``` | ||
| * | ||
| * @property closeTimeout the amount of time, in [closeTimeoutUnit] units, that should be waited | ||
| * when [close]ing this dispatcher before force-ending ongoing tasks | ||
| * @property closeTimeoutUnit the unit of time used for [closeTimeout] | ||
| */ | ||
| class ScriptBackgroundCoroutineDispatcher( | ||
| private val closeTimeout: Long = 5, | ||
| private val closeTimeoutUnit: TimeUnit = TimeUnit.SECONDS | ||
| ) : CoroutineDispatcher(), Closeable { | ||
| private val threadPool by lazy { Executors.newCachedThreadPool() } | ||
| private val coroutineDispatcher by lazy { threadPool.asCoroutineDispatcher() } | ||
|
|
||
| override fun dispatch(context: CoroutineContext, block: Runnable) { | ||
| coroutineDispatcher.dispatch(context, block) | ||
| } | ||
|
|
||
| override fun close() { | ||
| threadPool.tryShutdownFully(timeout = closeTimeout, unit = closeTimeoutUnit) | ||
| coroutineDispatcher.close() | ||
| } | ||
|
|
||
| private companion object { | ||
| private fun ExecutorService.tryShutdownFully(timeout: Long, unit: TimeUnit) { | ||
| // Try to fully shutdown the executor service per https://stackoverflow.com/a/33690603 and | ||
| // https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ExecutorService.html. | ||
| shutdown() | ||
| try { | ||
| if (!awaitTermination(timeout, unit)) { | ||
| shutdownNow() | ||
| check(awaitTermination(timeout, unit)) { "ExecutorService didn't fully shutdown: $this." } | ||
| } | ||
| } catch (e: InterruptedException) { | ||
| shutdownNow() | ||
| Thread.currentThread().interrupt() | ||
| } | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.