Search code examples
kotlinkotlin-coroutinessonarlint

Sonar issues a warning when i try to launch another suspend fun in a suspend fun


The function below runs correctly, but I don't understand why my Sonar plugin issues a warning for this line CoroutineScope(MDCContext()).launch { startPerformanceTest(request) }. Is this a false positive, and how can I eliminate it?

    override suspend fun startPerformance(request: PerformanceStartRequest) {
        if (!PerformanceTestLock.tryAcquireLock(request.taskId)) {
            throw BizException.of(RunnerException.SCRIPT_EXCLUSIVE)
        }

        CoroutineScope(MDCContext()).launch {
           // Remove this dispatcher. It is pointless when used with only suspending functions.
           // kotlin:S6311
            startPerformanceTest(request)
        }
    }

    private suspend fun startPerformanceTest(request: PerformanceStartRequest) = with(request) {
        try {
            val result = performanceDomainService.execute(taskId, script, runners)
                .apply {
                    instanceMetrics = metricDomainService.queryInstanceMetric(services, startTime, endTime)
                }
            messageDomainService.sendSuccessMessage(taskId, Phase.PERFORMANCE_TEST, result)
        } catch (e: Exception) {
            logger.error(e) { "Performance test failed!" }
            messageDomainService.sendFailureMessage(taskId, Phase.PERFORMANCE_TEST, e.message)
        } finally {
            PerformanceTestLock.releaseLock(taskId)
        }
    }

Solution

  • If you click on the issue you get the full explanation why this is problematic.

    To summarize: By convention, each suspending function is responsible to use an appropriate context/dispatcher itself. So it doesn't make sense to move the call of a suspending function to another dispatcher. That should be done inside the function, where it is actually needed.

    But there are more pressing issues with your code. Suspending functions should never, out of the blue, launch new coroutines. That violates the principles of structured concurrency. If it isn't sufficient to move the current coroutine where the function runs in to another thread (by simply using withContext()), you need to explicitly pass a CoroutineScope that can then be used instead of your ad-hoc scope CoroutineScope(MDCContext()) to launch a new coroutine. Maybe like this, as an extension function:

    fun CoroutineScope.startPerformance(request: PerformanceStartRequest) {
        if (!PerformanceTestLock.tryAcquireLock(request.taskId)) {
            throw BizException.of(RunnerException.SCRIPT_EXCLUSIVE)
        }
    
        launch {
            startPerformanceTest(request)
        }
    }
    

    This way the caller always has the power to cancel all coroutines by cancelling that scope. That won't be possible with a scope created by CoroutineScope. If the caller needs to create a new scope for this, use coroutineScope (starting with a lower c):

    suspend fun caller() {
        coroutineScope {
            // launch new coroutines here or call extension functions of CoroutineScope
            startPerformance(request)
        }
    }
    

    Note that coroutineScope blocks further execution until all coroutines that were launched in it are completed. This way the caller of caller does not need a reference to this scope, it can simply cancel the current coroutine because caller is still running there. That allows for automatic cancellation of the newly created scope as well.


    Curiously, in my current setup this will still raise the Sonar issue kotlin:S6311. That shouldn't be the case, because no dispatchers are involved anymore at that location. This seems to be a bug since even the Compliant solution shown in the link above still produces the same issue.