From 88c02feb448c4620750112fe10729926d735c3b2 Mon Sep 17 00:00:00 2001 From: Norbel AMBANUMBEN Date: Tue, 12 Nov 2024 16:38:05 +0100 Subject: [PATCH 1/2] fix: interrupt test in the engine --- .../src/commonMain/kotlin/org/ooni/engine/Engine.kt | 12 ++++++++++++ .../kotlin/org/ooni/probe/di/Dependencies.kt | 1 + .../kotlin/org/ooni/probe/domain/RunDescriptors.kt | 5 ++++- .../kotlin/org/ooni/probe/domain/RunNetTest.kt | 4 +++- .../commonTest/kotlin/org/ooni/engine/EngineTest.kt | 2 ++ 5 files changed, 22 insertions(+), 2 deletions(-) diff --git a/composeApp/src/commonMain/kotlin/org/ooni/engine/Engine.kt b/composeApp/src/commonMain/kotlin/org/ooni/engine/Engine.kt index 9f33c6f0..5c7184cd 100644 --- a/composeApp/src/commonMain/kotlin/org/ooni/engine/Engine.kt +++ b/composeApp/src/commonMain/kotlin/org/ooni/engine/Engine.kt @@ -3,9 +3,11 @@ package org.ooni.engine import androidx.annotation.VisibleForTesting import co.touchlab.kermit.Logger import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.async import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.channelFlow import kotlinx.coroutines.flow.flowOn +import kotlinx.coroutines.flow.take import kotlinx.coroutines.isActive import kotlinx.serialization.encodeToString import kotlinx.serialization.json.Json @@ -42,6 +44,7 @@ class Engine( inputs: List?, taskOrigin: TaskOrigin, descriptorId: InstalledTestDescriptorModel.Id?, + observeCancelTestRun: Flow, ): Flow = channelFlow { val preferences = getEnginePreferences() @@ -52,11 +55,20 @@ class Engine( try { task = bridge.startTask(settingsSerialized) + val cancelJob = async { + observeCancelTestRun + .take(1) + .collect { + task.interrupt() + } + } + while (!task.isDone() && isActive) { val eventJson = task.waitForNextEvent() val taskEventResult = json.decodeFromString(eventJson) taskEventMapper(taskEventResult)?.let { send(it) } } + cancelJob.cancel() } catch (e: CancellationException) { Logger.d("Test cancelled") throw e diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt index 6d99e0f8..3907327c 100644 --- a/composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt +++ b/composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt @@ -355,6 +355,7 @@ class Dependencies( deleteFiles = deleteFiles, json = json, spec = spec, + observeCancelTestRun = testStateManager.observeCancels(), ) // Background diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunDescriptors.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunDescriptors.kt index 4c92207b..c917d72d 100644 --- a/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunDescriptors.kt +++ b/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunDescriptors.kt @@ -68,6 +68,10 @@ class RunDescriptors( val runJob = async { // Actually running the descriptors descriptors.forEachIndexed { index, descriptor -> + // check if cancel has been requested before running descriptor + if (getCurrentTestRunState.first() is TestRunState.Stopping) { + return@forEachIndexed + } runDescriptor(descriptor, index, spec.taskOrigin, spec.isRerun) } } @@ -77,7 +81,6 @@ class RunDescriptors( .take(1) .collect { setCurrentTestState { TestRunState.Stopping } - runJob.cancel() } } diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunNetTest.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunNetTest.kt index aa74c225..c349bd46 100644 --- a/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunNetTest.kt +++ b/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunNetTest.kt @@ -21,7 +21,7 @@ import org.ooni.probe.data.models.UrlModel import org.ooni.probe.shared.toLocalDateTime class RunNetTest( - private val startTest: (String, List?, TaskOrigin, InstalledTestDescriptorModel.Id?) -> Flow, + private val startTest: (String, List?, TaskOrigin, InstalledTestDescriptorModel.Id?, Flow) -> Flow, private val getOrCreateUrl: suspend (String) -> UrlModel, private val storeMeasurement: suspend (MeasurementModel) -> MeasurementModel.Id, private val storeNetwork: suspend (NetworkModel) -> NetworkModel.Id, @@ -31,6 +31,7 @@ class RunNetTest( private val deleteFiles: DeleteFiles, private val json: Json, private val spec: Specification, + private val observeCancelTestRun: Flow, ) { data class Specification( val descriptor: Descriptor, @@ -68,6 +69,7 @@ class RunNetTest( spec.netTest.inputs, spec.taskOrigin, installedDescriptorId, + observeCancelTestRun, ) .collect(::onEvent) } diff --git a/composeApp/src/commonTest/kotlin/org/ooni/engine/EngineTest.kt b/composeApp/src/commonTest/kotlin/org/ooni/engine/EngineTest.kt index 22c07a29..5b63da1f 100644 --- a/composeApp/src/commonTest/kotlin/org/ooni/engine/EngineTest.kt +++ b/composeApp/src/commonTest/kotlin/org/ooni/engine/EngineTest.kt @@ -1,6 +1,7 @@ package org.ooni.engine import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.emptyFlow import kotlinx.coroutines.flow.toList import kotlinx.coroutines.test.runTest import org.ooni.engine.models.EnginePreferences @@ -33,6 +34,7 @@ class EngineTest { inputs = listOf("https://ooni.org"), taskOrigin = TaskOrigin.OoniRun, descriptorId = null, + observeCancelTestRun = emptyFlow(), ).toList() assertEquals(1, events.size) From 16ac9a6e930076f9c5803876ae1b594d03b0cbc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Santos?= Date: Tue, 12 Nov 2024 17:00:50 +0000 Subject: [PATCH 2/2] Tweak run stopping --- .../kotlin/org/ooni/engine/Engine.kt | 17 +++++++++-------- .../kotlin/org/ooni/probe/di/Dependencies.kt | 2 +- .../org/ooni/probe/domain/RunDescriptors.kt | 19 ++++++++----------- .../org/ooni/probe/domain/RunNetTest.kt | 4 +--- .../kotlin/org/ooni/engine/EngineTest.kt | 2 +- 5 files changed, 20 insertions(+), 24 deletions(-) diff --git a/composeApp/src/commonMain/kotlin/org/ooni/engine/Engine.kt b/composeApp/src/commonMain/kotlin/org/ooni/engine/Engine.kt index 5c7184cd..4e6dba0a 100644 --- a/composeApp/src/commonMain/kotlin/org/ooni/engine/Engine.kt +++ b/composeApp/src/commonMain/kotlin/org/ooni/engine/Engine.kt @@ -2,7 +2,6 @@ package org.ooni.engine import androidx.annotation.VisibleForTesting import co.touchlab.kermit.Logger -import kotlinx.coroutines.CancellationException import kotlinx.coroutines.async import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.channelFlow @@ -37,6 +36,7 @@ class Engine( private val isBatteryCharging: suspend () -> Boolean, private val platformInfo: PlatformInfo, private val getEnginePreferences: suspend () -> EnginePreferences, + private val observeCancelTestRun: () -> Flow, private val backgroundContext: CoroutineContext, ) { fun startTask( @@ -44,7 +44,6 @@ class Engine( inputs: List?, taskOrigin: TaskOrigin, descriptorId: InstalledTestDescriptorModel.Id?, - observeCancelTestRun: Flow, ): Flow = channelFlow { val preferences = getEnginePreferences() @@ -56,7 +55,7 @@ class Engine( task = bridge.startTask(settingsSerialized) val cancelJob = async { - observeCancelTestRun + observeCancelTestRun() .take(1) .collect { task.interrupt() @@ -68,15 +67,17 @@ class Engine( val taskEventResult = json.decodeFromString(eventJson) taskEventMapper(taskEventResult)?.let { send(it) } } - cancelJob.cancel() - } catch (e: CancellationException) { - Logger.d("Test cancelled") - throw e + + if (cancelJob.isActive) { + cancelJob.cancel() + } } catch (e: Exception) { Logger.d("Error while running task", e) throw MkException(e) } finally { - task?.interrupt() + if (task?.isDone() == false) { + task.interrupt() + } } }.flowOn(backgroundContext) diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt index 3907327c..80d1e277 100644 --- a/composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt +++ b/composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt @@ -183,6 +183,7 @@ class Dependencies( isBatteryCharging = isBatteryCharging, platformInfo = platformInfo, getEnginePreferences = getEnginePreferences::invoke, + observeCancelTestRun = testStateManager::observeCancels, backgroundContext = backgroundContext, ) } @@ -355,7 +356,6 @@ class Dependencies( deleteFiles = deleteFiles, json = json, spec = spec, - observeCancelTestRun = testStateManager.observeCancels(), ) // Background diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunDescriptors.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunDescriptors.kt index c917d72d..5b4ae4b3 100644 --- a/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunDescriptors.kt +++ b/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunDescriptors.kt @@ -65,16 +65,6 @@ class RunDescriptors( spec: RunSpecification, ) { coroutineScope { - val runJob = async { - // Actually running the descriptors - descriptors.forEachIndexed { index, descriptor -> - // check if cancel has been requested before running descriptor - if (getCurrentTestRunState.first() is TestRunState.Stopping) { - return@forEachIndexed - } - runDescriptor(descriptor, index, spec.taskOrigin, spec.isRerun) - } - } // Observe if a cancel request has been made val cancelJob = async { observeCancelTestRun @@ -84,7 +74,11 @@ class RunDescriptors( } } - runJob.await() + // Actually running the descriptors + descriptors.forEachIndexed { index, descriptor -> + if (isRunStopped()) return@forEachIndexed + runDescriptor(descriptor, index, spec.taskOrigin, spec.isRerun) + } if (cancelJob.isActive) { cancelJob.cancel() @@ -142,6 +136,7 @@ class RunDescriptors( val resultId = storeResult(result) descriptor.allTests.forEachIndexed { testIndex, netTest -> + if (isRunStopped()) return@forEachIndexed runNetTest( RunNetTest.Specification( descriptor = descriptor, @@ -158,4 +153,6 @@ class RunDescriptors( markResultAsDone(resultId) } + + private suspend fun isRunStopped() = getCurrentTestRunState.first() is TestRunState.Stopping } diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunNetTest.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunNetTest.kt index c349bd46..aa74c225 100644 --- a/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunNetTest.kt +++ b/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunNetTest.kt @@ -21,7 +21,7 @@ import org.ooni.probe.data.models.UrlModel import org.ooni.probe.shared.toLocalDateTime class RunNetTest( - private val startTest: (String, List?, TaskOrigin, InstalledTestDescriptorModel.Id?, Flow) -> Flow, + private val startTest: (String, List?, TaskOrigin, InstalledTestDescriptorModel.Id?) -> Flow, private val getOrCreateUrl: suspend (String) -> UrlModel, private val storeMeasurement: suspend (MeasurementModel) -> MeasurementModel.Id, private val storeNetwork: suspend (NetworkModel) -> NetworkModel.Id, @@ -31,7 +31,6 @@ class RunNetTest( private val deleteFiles: DeleteFiles, private val json: Json, private val spec: Specification, - private val observeCancelTestRun: Flow, ) { data class Specification( val descriptor: Descriptor, @@ -69,7 +68,6 @@ class RunNetTest( spec.netTest.inputs, spec.taskOrigin, installedDescriptorId, - observeCancelTestRun, ) .collect(::onEvent) } diff --git a/composeApp/src/commonTest/kotlin/org/ooni/engine/EngineTest.kt b/composeApp/src/commonTest/kotlin/org/ooni/engine/EngineTest.kt index 5b63da1f..30dd91fa 100644 --- a/composeApp/src/commonTest/kotlin/org/ooni/engine/EngineTest.kt +++ b/composeApp/src/commonTest/kotlin/org/ooni/engine/EngineTest.kt @@ -34,7 +34,6 @@ class EngineTest { inputs = listOf("https://ooni.org"), taskOrigin = TaskOrigin.OoniRun, descriptorId = null, - observeCancelTestRun = emptyFlow(), ).toList() assertEquals(1, events.size) @@ -87,6 +86,7 @@ class EngineTest { maxRuntime = null, ) }, + observeCancelTestRun = { emptyFlow() }, backgroundContext = Dispatchers.Unconfined, ) }