both Stop and Disconnected event goes to a ClientClosed state and bro…

…adcast a Closed connection state (#176) * Revert "Fix electrum client doubling 'Disconnected' event (#173)" This reverts commit 8876962 * both Stop and Disconnected event goes to a ClientClosed state and broadcast a Closed connection state (fixes #174)
ACINQ · Jan 22, 2021 · abc3c79 · abc3c79
1 parent 8d4f5b5
commit abc3c79
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 20 deletions.
diff --git a/src/commonMain/kotlin/fr/acinq/eclair/blockchain/electrum/ElectrumClient.kt b/src/commonMain/kotlin/fr/acinq/eclair/blockchain/electrum/ElectrumClient.kt
@@ -27,9 +27,9 @@ import kotlin.time.seconds
  */
 internal sealed class ClientEvent
 internal data class Start(val serverAddress: ServerAddress) : ClientEvent()
+internal object Stop : ClientEvent()
 internal object Connected : ClientEvent()
 internal object Disconnected : ClientEvent()
-internal object Close : ClientEvent()
 internal data class ReceivedResponse(val response: Either<ElectrumResponse, JsonRPCResponse>) : ClientEvent()
 internal data class SendElectrumApiCall(val electrumRequest: ElectrumRequest) : ClientEvent()
 internal object AskForStatus : ClientEvent()
@@ -157,14 +157,10 @@ internal object ClientClosed : ClientState() {
 
 private fun ClientState.unhandled(event: ClientEvent): Pair<ClientState, List<ElectrumClientAction>> =
     when (event) {
-        Disconnected -> newState {
+        Stop, Disconnected -> newState {
             state = ClientClosed
             actions = listOf(BroadcastStatus(Connection.CLOSED), Shutdown)
         }
-        Close -> newState {
-            state = ClientClosed
-            actions = listOf(BroadcastStatus(Connection.CLOSED))
-        }
         AskForStatus, AskForHeader -> returnState() // TODO something else ?
         else -> {
             logger.warning { "cannot process event ${event::class} in state ${this::class}" }
@@ -233,7 +229,7 @@ class ElectrumClient(
                     is SendResponse -> notificationsChannel.send(action.response)
                     is BroadcastStatus -> _connectionState.value = action.connection
                     StartPing -> pingJob = pingScheduler()
-                    is Shutdown -> disconnect()
+                    is Shutdown -> cancelSocketUsage()
                 }
             }
         }
@@ -245,9 +241,7 @@ class ElectrumClient(
     }
 
     fun disconnect() {
-        pingJob?.cancel()
-        connectionJob?.cancel() ?: launch { eventChannel.send(Close) }
-        if (this::socket.isInitialized) socket.close()
+        launch { eventChannel.send(Stop) }
     }
 
     private var connectionJob: Job? = null
@@ -265,9 +259,15 @@ class ElectrumClient(
         } catch (ex: TcpSocket.IOException) {
             logger.warning { ex.message }
             eventChannel.send(Disconnected)
+            socket.close()
         }
     }
 
+    private fun cancelSocketUsage() {
+        pingJob?.cancel()
+        connectionJob?.cancel()
+    }
+
     private suspend fun send(message: ByteArray) {
         try {
             socket.send(message)
@@ -299,7 +299,7 @@ class ElectrumClient(
 
     fun stop() {
         logger.info { "electrum client stopping" }
-        disconnect()
+        cancelSocketUsage()
         // Cancel event consumer
         runJob?.cancel()
         // Cancel broadcast channels

diff --git a/src/commonTest/kotlin/fr/acinq/eclair/blockchain/electrum/ElectrumClientStateTest.kt b/src/commonTest/kotlin/fr/acinq/eclair/blockchain/electrum/ElectrumClientStateTest.kt
@@ -3,6 +3,7 @@ package fr.acinq.eclair.blockchain.electrum
 import fr.acinq.bitcoin.BlockHeader
 import fr.acinq.eclair.io.TcpSocket
 import fr.acinq.eclair.tests.utils.EclairTestSuite
+import fr.acinq.eclair.utils.Connection
 import fr.acinq.eclair.utils.ServerAddress
 import kotlin.test.Test
 import kotlin.test.assertEquals
@@ -73,16 +74,26 @@ class ElectrumClientStateTest : EclairTestSuite() {
 
     @Test
     fun `unhandled events`() {
-        val states = listOf(
-            ClientRunning(0, testBlockHeader), WaitingForVersion, WaitingForTip, ClientClosed
-        )
-        states.forEach { state ->
-            state.process(Disconnected).let { (newState, actions) ->
-                assertEquals(ClientClosed, newState)
-                assertEquals(2, actions.size)
-                assertTrue { actions[0] is BroadcastStatus }
-                assertTrue { actions[1] is Shutdown }
+        listOf(
+            WaitingForConnection, WaitingForVersion, WaitingForTip, ClientRunning(0, testBlockHeader), ClientClosed
+        ).forEach { state ->
+            listOf(Stop, Disconnected).forEach { event ->
+                state.process(event).let { (nextState, actions) ->
+                    assertEquals(ClientClosed, nextState)
+                    assertEquals(2, actions.size)
+                    assertTrue(actions[0] is BroadcastStatus)
+                    assertEquals(Connection.CLOSED, (actions[0] as BroadcastStatus).connection)
+                    assertTrue(actions[1] is Shutdown)
+                }
             }
+
+            if (state !is ClientRunning)
+                listOf(AskForStatus, AskForHeader).forEach { event ->
+                    state.process(event).let { (nextState, actions) ->
+                        assertEquals(state, nextState)
+                        assertTrue(actions.isEmpty())
+                    }
+                }
         }
     }
 }