[console] Fix plugins disabling

mirai-console/backend/integration-test/test/testpoints/plugin/PluginOnDisableCalledOnlyOnceTest.kt

@@ -0,0 +1,33 @@
+/*
+ * Copyright 2019-2022 Mamoe Technologies and contributors.
+ *
+ * 此源代码的使用受 GNU AFFERO GENERAL PUBLIC LICENSE version 3 许可证的约束, 可以在以下链接找到该许可证.
+ * Use of this source code is governed by the GNU AGPLv3 license that can be found through the following link.
+ *
+ * https://github.com/mamoe/mirai/blob/dev/LICENSE
+ */
+
+package net.mamoe.console.integrationtest.testpoints.plugin
+
+import kotlinx.atomicfu.atomic
+import net.mamoe.console.integrationtest.AbstractTestPointAsPlugin
+import net.mamoe.mirai.console.plugin.jvm.JvmPluginDescription
+import net.mamoe.mirai.console.plugin.jvm.KotlinPlugin
+
+internal object PluginOnDisableCalledOnlyOnceTest : AbstractTestPointAsPlugin() {
+    override fun newPluginDescription(): JvmPluginDescription {
+        return JvmPluginDescription(
+            id = "net.mamoe.testpoint.plugin-on-disable-called-only-once",
+            version = "1.0.0",
+            name = "PluginOnDisableCalledOnlyOnce",
+        ) {}
+    }
+
+    private val onDisableCalled = atomic(false)
+
+    override fun KotlinPlugin.onDisable0() {
+        if (!onDisableCalled.compareAndSet(expect = false, update = true)) {
+            error("onDisable called multiple times!!")
+        }
+    }
+}

mirai-console/backend/mirai-console/src/MiraiConsoleImplementation.kt

@@ -29,6 +29,7 @@ import net.mamoe.mirai.console.internal.command.CommandManagerImpl
 import net.mamoe.mirai.console.internal.data.builtins.ConsoleDataScopeImpl
 import net.mamoe.mirai.console.internal.logging.LoggerControllerImpl
 import net.mamoe.mirai.console.internal.plugin.BuiltInJvmPluginLoaderImpl
+import net.mamoe.mirai.console.internal.plugin.impl
 import net.mamoe.mirai.console.internal.pluginManagerImpl
 import net.mamoe.mirai.console.logging.LoggerController
 import net.mamoe.mirai.console.plugin.Plugin
@@ -350,7 +351,7 @@ public interface MiraiConsoleImplementation : CoroutineScope {
          * @since 2.10.0-RC
          */
         public fun createDefaultJvmPluginLoader(coroutineContext: CoroutineContext): JvmPluginLoader =
-            BuiltInJvmPluginLoaderImpl(coroutineContext)
+            BuiltInJvmPluginLoaderImpl(coroutineContext + MiraiConsole.pluginManager.impl.coroutineContext.job)
 
         /**
          * @since 2.10.0-RC

mirai-console/backend/mirai-console/src/command/BuiltInCommands.kt

@@ -35,6 +35,7 @@ import net.mamoe.mirai.console.internal.data.builtins.DataScope
 import net.mamoe.mirai.console.internal.extension.GlobalComponentStorage
 import net.mamoe.mirai.console.internal.permission.BuiltInPermissionService
 import net.mamoe.mirai.console.internal.permission.getPermittedPermissionsAndSource
+import net.mamoe.mirai.console.internal.plugin.JvmPluginInternal
 import net.mamoe.mirai.console.internal.pluginManagerImpl
 import net.mamoe.mirai.console.internal.util.runIgnoreException
 import net.mamoe.mirai.console.permission.Permission
@@ -617,7 +618,13 @@ public object BuiltInCommands {
                         if (plugin.isEnabled) {
                             green().append(plugin.name).reset().append(" v").gold()
                         } else {
-                            red().append(plugin.name).append("(disabled)").reset().append(" v").gold()
+                            red().append(plugin.name)
+                            if (plugin is JvmPluginInternal) {
+                                append("(").append(plugin.currentPluginStatus.name.lowercase()).append(")")
+                            } else {
+                                append("(disabled)")
+                            }
+                            reset().append(" v").gold()
                         }
                         plugin.version.toString()
                     }

mirai-console/backend/mirai-console/src/internal/plugin/JvmPluginInternal.kt

@@ -10,6 +10,7 @@
 package net.mamoe.mirai.console.internal.plugin
 
 import kotlinx.atomicfu.AtomicLong
+import kotlinx.atomicfu.atomic
 import kotlinx.atomicfu.locks.withLock
 import kotlinx.coroutines.*
 import net.mamoe.mirai.console.MiraiConsole
@@ -25,6 +26,7 @@ import net.mamoe.mirai.console.plugin.Plugin
 import net.mamoe.mirai.console.plugin.PluginManager
 import net.mamoe.mirai.console.plugin.PluginManager.INSTANCE.safeLoader
 import net.mamoe.mirai.console.plugin.ResourceContainer.Companion.asResourceContainer
+import net.mamoe.mirai.console.plugin.id
 import net.mamoe.mirai.console.plugin.jvm.AbstractJvmPlugin
 import net.mamoe.mirai.console.plugin.jvm.JvmPlugin
 import net.mamoe.mirai.console.plugin.jvm.JvmPlugin.Companion.onLoad
@@ -47,6 +49,73 @@ internal val <T> T.job: Job where T : CoroutineScope, T : Plugin get() = this.co
 internal abstract class JvmPluginInternal(
     parentCoroutineContext: CoroutineContext,
 ) : JvmPlugin, CoroutineScope {
+    internal enum class PluginStatus {
+        ALLOCATED,
+
+        CRASHED_LOAD_ERROR(Flags.ALLOW_SWITCH_TO_DISABLE),
+        CRASHED_ENABLE_ERROR(Flags.ALLOW_SWITCH_TO_DISABLE),
+        CRASHED_DISABLE_ERROR,
+
+        LOAD_PENDING,
+        LOAD_LOADING,
+        LOAD_LOAD_DONE,
+
+        ENABLE_PENDING,
+        ENABLE_ENABLING,
+        ENABLED(Flags.ALLOW_SWITCH_TO_DISABLE),
+
+        DISABLE_PENDING,
+        DISABLE_DISABLING,
+        DISABLED,
+
+        ;
+
+        private val flags: Int
+
+        constructor() : this(0)
+        constructor(flags: Int) {
+            this.flags = flags
+        }
+
+        internal object Flags { // compiler bug: [UNINITIALIZED_VARIABLE] Variable 'FLAG_ALLOW_SWITCH_TO_DISABLE' must be initialized
+            internal const val ALLOW_SWITCH_TO_DISABLE = 1 shl 0
+        }
+
+        fun hasFlag(flag: Int): Boolean = flags.and(flag) != 0
+    }
+
+    private val pluginStatus = atomic(PluginStatus.ALLOCATED)
+
+    @get:JvmSynthetic
+    internal val currentPluginStatus: PluginStatus get() = pluginStatus.value
+
+    final override val isEnabled: Boolean
+        get() = pluginStatus.value === PluginStatus.ENABLED
+
+    @JvmSynthetic
+    internal fun switchStatusOrFail(expectFlag: Int, update: PluginStatus) {
+        val nowStatus = pluginStatus.value
+        if (nowStatus.hasFlag(expectFlag)) {
+            if (pluginStatus.compareAndSet(expect = nowStatus, update = update)) {
+                return
+            }
+            error("Failed to switch plugin '$id' status from $nowStatus to $update, current status = ${pluginStatus.value}")
+        }
+        error("Failed to switch plugin '$id' status to $update because current status $nowStatus doesn't contain flag ${Integer.toBinaryString(expectFlag)}")
+    }
+
+    @JvmSynthetic
+    internal fun switchStatusOrFail(expect: PluginStatus, update: PluginStatus) {
+        val nowStatus = pluginStatus.value
+        if (nowStatus === expect) {
+            if (pluginStatus.compareAndSet(expect = expect, update = update)) {
+                return
+            }
+            error("Failed to switch plugin '$id' status from $expect to $update, current status=${pluginStatus.value}")
+        }
+        error("Failed to switch plugin '$id' status from $expect to $update, current status = $nowStatus")
+    }
+
 
     final override val parentPermission: Permission by lazy {
         PermissionService.INSTANCE.register(
@@ -55,8 +124,6 @@ internal abstract class JvmPluginInternal(
         )
     }
 
-    final override var isEnabled: Boolean = false
-        internal set
 
     private val resourceContainerDelegate by lazy { this::class.java.classLoader.asResourceContainer() }
     final override fun getResourceAsStream(path: String): InputStream? =
@@ -88,20 +155,31 @@ internal abstract class JvmPluginInternal(
     }
 
     internal fun internalOnDisable() {
+
+        switchStatusOrFail(
+            expectFlag = PluginStatus.Flags.ALLOW_SWITCH_TO_DISABLE,
+            update = PluginStatus.DISABLE_PENDING,
+        )
+
         firstRun = false
         kotlin.runCatching {
             val crtThread = Thread.currentThread()
             ShutdownDaemon.pluginDisablingThreads.add(crtThread)
             try {
+                pluginStatus.value = PluginStatus.DISABLE_DISABLING
                 onDisable()
             } finally {
                 ShutdownDaemon.pluginDisablingThreads.remove(crtThread)
             }
         }.fold(
             onSuccess = {
+                pluginStatus.value = PluginStatus.DISABLED
+
                 cancel(CancellationException("plugin disabled"))
             },
             onFailure = { err ->
+                pluginStatus.value = PluginStatus.CRASHED_DISABLE_ERROR
+
                 cancel(CancellationException("Exception while disabling plugin", err))
 
                 // @TestOnly
@@ -112,17 +190,32 @@ internal abstract class JvmPluginInternal(
                 }
             }
         )
-        isEnabled = false
     }
 
     @Throws(Throwable::class)
     internal fun internalOnLoad() {
-        val componentStorage = PluginComponentStorage(this)
-        onLoad(componentStorage)
-        GlobalComponentStorage.mergeWith(componentStorage)
+        switchStatusOrFail(PluginStatus.ALLOCATED, PluginStatus.LOAD_PENDING)
+
+        try {
+            pluginStatus.value = PluginStatus.LOAD_LOADING
+
+            val componentStorage = PluginComponentStorage(this)
+            onLoad(componentStorage)
+
+            pluginStatus.value = PluginStatus.LOAD_LOAD_DONE
+            GlobalComponentStorage.mergeWith(componentStorage)
+
+        } catch (e: Throwable) {
+            pluginStatus.value = PluginStatus.CRASHED_LOAD_ERROR
+
+            cancel(CancellationException("Exception while loading plugin", e))
+        }
     }
 
+
     internal fun internalOnEnable(): Boolean {
+        switchStatusOrFail(PluginStatus.LOAD_LOAD_DONE, PluginStatus.ENABLE_PENDING)
+
         parentPermission
         if (!firstRun) refreshCoroutineContext()
 
@@ -133,6 +226,7 @@ internal abstract class JvmPluginInternal(
         }
 
         kotlin.runCatching {
+            pluginStatus.value = PluginStatus.ENABLE_ENABLING
             onEnable()
         }.fold(
             onSuccess = {
@@ -142,10 +236,12 @@ internal abstract class JvmPluginInternal(
                     logger.error(msg)
                     throw AssertionError(msg)
                 }
-                isEnabled = true
+                pluginStatus.value = PluginStatus.ENABLED
                 return true
             },
             onFailure = { err ->
+                pluginStatus.value = PluginStatus.CRASHED_ENABLE_ERROR
+
                 cancel(CancellationException("Exception while enabling plugin", err))
                 logger.error(err)
 
@@ -189,12 +285,30 @@ internal abstract class JvmPluginInternal(
 
     // for future use
     @Suppress("PropertyName")
+    @get:JvmSynthetic
     internal val _intrinsicCoroutineContext: CoroutineContext by lazy {
         this as AbstractJvmPlugin
         CoroutineName("Plugin $dataHolderName")
     }
 
+    private val pluginParentJob: Job = run {
+        val job = parentCoroutineContext[Job] ?: JvmPluginLoader.coroutineContext[Job]!!
+
+        val pluginManagerJob = MiraiConsole.pluginManager.impl.coroutineContext.job
+
+        val allJobs = generateSequence(sequenceOf(pluginManagerJob)) { parentSeqs ->
+            parentSeqs.flatMap { it.children }
+        }.flatten()
+
+        check(allJobs.contains(job)) {
+            "The parent job of plugin `$id' not a child of PluginManager"
+        }
+
+        job
+    }
+
     @JvmField
+    @JvmSynthetic
     internal val coroutineContextInitializer = {
         CoroutineExceptionHandler { context, throwable ->
             if (throwable.rootCauseOrSelf !is CancellationException) logger.error(
@@ -203,16 +317,7 @@ internal abstract class JvmPluginInternal(
             )
         }
             .plus(parentCoroutineContext)
-            .plus(CoroutineName("Plugin ${(this as AbstractJvmPlugin).dataHolderName}"))
-            .plus(
-                SupervisorJob(parentCoroutineContext[Job] ?: JvmPluginLoader.coroutineContext[Job]!!)
-            )
-            .also {
-                if (!MiraiConsole.isActive) return@also
-                JvmPluginLoader.coroutineContext[Job]!!.invokeOnCompletion {
-                    this.cancel()
-                }
-            }
+            .plus(SupervisorJob(pluginParentJob))
             .plus(_intrinsicCoroutineContext)
     }
 
@@ -221,7 +326,9 @@ internal abstract class JvmPluginInternal(
             job.invokeOnCompletion { e ->
                 if (e != null) {
                     if (e !is CancellationException) logger.error(e)
-                    if (this.isEnabled) safeLoader.disable(this)
+                    if (pluginStatus.value == PluginStatus.ENABLED) {
+                        safeLoader.disable(this)
+                    }
                 }
             }
         }

mirai-console/backend/mirai-console/src/internal/plugin/PluginManagerImpl.kt

@@ -12,7 +12,7 @@
 package net.mamoe.mirai.console.internal.plugin
 
 import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.Job
+import kotlinx.coroutines.job
 import net.mamoe.mirai.console.MiraiConsole
 import net.mamoe.mirai.console.extensions.PluginLoaderProvider
 import net.mamoe.mirai.console.internal.data.mkdir
@@ -74,7 +74,13 @@ internal class PluginManagerImpl(
         plugin.safeLoader.getPluginDescription(plugin)
 
     init {
-        MiraiConsole.coroutineContext[Job]!!.invokeOnCompletion {
+        // Kotlin coroutine job cancelling ordering:
+        // - sub job 0 invokeOnCompletion called
+        // - sub job 1 invokeOnCompletion called
+        // - sub job N invokeOnCompletion called
+        // - parent    invokeOnCompletion called
+        // So we need register a child job to control plugins' disabling order
+        this.childScopeContext("PluginManager shutdown monitor").job.invokeOnCompletion {
             plugins.asReversed().forEach { plugin ->
                 if (plugin.isEnabled) {
                     disablePlugin(plugin)

mirai-console/backend/mirai-console/test/data/PluginMovingTests.kt

@@ -21,11 +21,17 @@ import net.mamoe.mirai.console.testFramework.AbstractConsoleInstanceTest
 import kotlin.test.Test
 
 class PluginMovingTests : AbstractConsoleInstanceTest() {
-    private val mockPluginWithName = object : KotlinPlugin(JvmPluginDescription("org.test1.test1", "1.0.0", "test1")) {}
-    private val mockPluginWithName2 =
+    private val mockPluginWithName by lazy {
+        object : KotlinPlugin(JvmPluginDescription("org.test1.test1", "1.0.0", "test1")) {}
+    }
+
+    private val mockPluginWithName2 by lazy {
         object : KotlinPlugin(JvmPluginDescription("org.test2.test2", "1.0.0", "test2")) {}
-    private val mockPluginWithName3 =
+    }
+
+    private val mockPluginWithName3 by lazy {
         object : KotlinPlugin(JvmPluginDescription("org.test2.test3", "1.0.0", "test3")) {}
+    }
 
     private fun mkdir(abstractPath: String) = PluginManager.pluginsDataPath.resolve(abstractPath).mkdir()