Always throw MaxAttemptsReachedException when for consistency;

Correctly count failure attempts;
Tolerant `NetworkException` thrown while `resumeInstanceCatchingException`, fix #1361

mirai-core/src/commonMain/kotlin/network/handler/selector/AbstractKeepAliveNetworkHandlerSelector.kt

@@ -10,10 +10,7 @@
 package net.mamoe.mirai.internal.network.handler.selector
 
 import kotlinx.atomicfu.atomic
-import kotlinx.coroutines.currentCoroutineContext
-import kotlinx.coroutines.delay
-import kotlinx.coroutines.isActive
-import kotlinx.coroutines.yield
+import kotlinx.coroutines.*
 import net.mamoe.mirai.internal.network.handler.NetworkHandler
 import net.mamoe.mirai.internal.network.handler.NetworkHandlerFactory
 import net.mamoe.mirai.internal.network.handler.logger
@@ -85,30 +82,50 @@ internal abstract class AbstractKeepAliveNetworkHandlerSelector<H : NetworkHandl
         private tailrec suspend fun runImpl(): H {
             if (attempted >= maxAttempts) {
                 logIfEnabled { "Max attempt $maxAttempts reached." }
-                throw exceptionCollector.getLast()?.apply { addSuppressed(MaxAttemptsReachedException(null)) }
-                    ?: MaxAttemptsReachedException(null)
+                throw MaxAttemptsReachedException(exceptionCollector.getLast())
+//                throw exceptionCollector.getLast()?.apply { addSuppressed(MaxAttemptsReachedException(null)) }
+//                    ?: MaxAttemptsReachedException(null)
             }
             if (!currentCoroutineContext().isActive) {
                 logIfEnabled { "Cancellation detected." }
                 yield() // throw canonical CancellationException if cancelled
+                throw CancellationException("Cancellation detected.")
             }
             val current = getCurrentInstanceOrNull()
             lastNetwork = current
 
-            suspend fun H.resumeInstanceCatchingException() {
+            /**
+             * @return `false` if failed
+             */
+            suspend fun H.resumeInstanceCatchingException(): Boolean {
                 logIfEnabled { "Try resumeConnection" }
                 try {
                     resumeConnection() // once finished, it should has been LOADING or OK
+                    return true
                 } catch (e: LoginFailedException) {
                     logIfEnabled { "... failed with LoginFailedException $e" }
                     logIfEnabled { "CLOSING SELECTOR" }
                     close(e)
                     logIfEnabled { "... CLOSED" }
+                    exceptionCollector.collect(e)
                     if (e is RetryLaterException) {
-                        return
+                        return false
                     }
                     // LoginFailedException is not resumable
-                    exceptionCollector.collectThrow(e)
+                    exceptionCollector.throwLast()
+                } catch (e: NetworkException) {
+                    logIfEnabled { "... failed with NetworkException $e" }
+                    logIfEnabled { "... recoverable=${e.recoverable}" }
+                    exceptionCollector.collect(e)
+                    if (e.recoverable) {
+                        logIfEnabled { "IGNORING" }
+                        // allow recoverable NetworkException, see #1361
+                    } else {
+                        logIfEnabled { "CLOSING SELECTOR" }
+                        close(e)
+                        logIfEnabled { "... CLOSED" }
+                    }
+                    return false
                 } catch (e: Exception) {
                     logIfEnabled { "... failed with $e" }
                     logIfEnabled { "CLOSING SELECTOR" }
@@ -116,6 +133,7 @@ internal abstract class AbstractKeepAliveNetworkHandlerSelector<H : NetworkHandl
                     logIfEnabled { "... CLOSED" }
                     // exception will be collected by `exceptionCollector.collectException(current.getLastFailure())`
                     // so that duplicated exceptions are ignored in logging
+                    return false
                 }
             }
 
@@ -141,8 +159,10 @@ internal abstract class AbstractKeepAliveNetworkHandlerSelector<H : NetworkHandl
                     NetworkHandler.State.CONNECTING,
                     NetworkHandler.State.INITIALIZED,
                     -> {
-                        current.resumeInstanceCatchingException()
-                        return runImpl() // does not count for an attempt.
+                        if (!current.resumeInstanceCatchingException()) {
+                            attempted += 1
+                        }
+                        return runImpl()
                     }
                     NetworkHandler.State.LOADING -> {
                         logIfEnabled { "RETURN" }

mirai-core/src/commonTest/kotlin/network/handler/KeepAliveNetworkHandlerSelectorRealTest.kt

@@ -14,38 +14,62 @@ package net.mamoe.mirai.internal.network.handler
 import io.netty.channel.Channel
 import net.mamoe.mirai.internal.network.framework.AbstractNettyNHTest
 import net.mamoe.mirai.internal.network.framework.TestNettyNH
+import net.mamoe.mirai.internal.network.handler.selector.MaxAttemptsReachedException
+import net.mamoe.mirai.internal.network.handler.selector.NetworkException
 import net.mamoe.mirai.internal.test.runBlockingUnit
 import net.mamoe.mirai.utils.TestOnly
 import org.junit.jupiter.api.assertThrows
-import java.net.SocketAddress
 import kotlin.test.Test
+import kotlin.test.assertIs
 
 internal class KeepAliveNetworkHandlerSelectorRealTest : AbstractNettyNHTest() {
 
     internal class FakeFailOnCreatingConnection : AbstractNettyNHTest() {
         private class MyException : Exception()
 
-        override val factory: NetworkHandlerFactory<TestNettyNH> = object : NetworkHandlerFactory<TestNettyNH> {
-            override fun create(context: NetworkHandlerContext, address: SocketAddress): TestNettyNH {
-                return object : TestNettyNH(bot, context, address) {
+        private lateinit var throwException: () -> Nothing
+
+        override val factory: NetworkHandlerFactory<TestNettyNH> =
+            NetworkHandlerFactory<TestNettyNH> { context, address ->
+                object : TestNettyNH(bot, context, address) {
                     override suspend fun createConnection(decodePipeline: PacketDecodePipeline): Channel =
-                        throw MyException()
+                        throwException()
                 }
             }
-        }
 
         @Test
-        fun `should not tolerant any exception thrown by states`() = runBlockingUnit {
+        fun `should not tolerant any exception except NetworkException thrown by states`() = runBlockingUnit {
             // selector should not tolerant any exception during state initialization, or in the Jobs launched in states.
+            throwException = {
+                throw MyException()
+            }
 
             val selector = TestSelector(3) { createHandler() }
             assertThrows<Throwable> { selector.awaitResumeInstance() }
         }
 
         @Test
-        fun `should unwrap exception`() = runBlockingUnit {
+        fun `should tolerant NetworkException thrown by states`() = runBlockingUnit {
+            // selector should not tolerant any exception during state initialization, or in the Jobs launched in states.
+            throwException = {
+                throw object : NetworkException(true) {}
+            }
+
             val selector = TestSelector(3) { createHandler() }
-            assertThrows<MyException> { selector.awaitResumeInstance() }
+            assertThrows<MaxAttemptsReachedException> { selector.awaitResumeInstance() }.let {
+                assertIs<NetworkException>(it.cause)
+            }
+        }
+
+        @Test
+        fun `throws MaxAttemptsReachedException with cause of original`() = runBlockingUnit {
+            throwException = {
+                throw MyException()
+            }
+            val selector = TestSelector(3) { createHandler() }
+            assertThrows<MaxAttemptsReachedException> { selector.awaitResumeInstance() }.let {
+                assertIs<MyException>(it.cause)
+            }
         }
     }