From 06a20eb7666bfac877c1babeb4b07f48d982c860 Mon Sep 17 00:00:00 2001
From: Ostap Maliuvanchuk <ostap@salemove.com>
Date: Wed, 20 Feb 2019 20:21:14 +0200
Subject: [PATCH] Fix isConnected

Currently `isConnected` is true when `connection` exists. However, this
is different from JS and Swift implementations.

In [JS][1] and [Swift][2] implementations `isConnected` is true only when the
socket is actually connected.

One of the problems this causes was the channels were closed during the
following scenario:
1. WebSocket connection established, one channel is joined..
2. Internet/Wifi disabled -> connection fails.
3. We periodically call `phxSocket.connect()` to reconnect.
4. Channels [try to rejoin as `isConnected` is true][4], but `phx_join`
messages are being queued instead as connection is still not established.
5. Internet/Wifi enabled -> multiple queued `phx_join` messages are sent,
Phoniex Server relies `phx_close` to all join messages but last.

On `phx_close` the channel is removed from the socket, even tho the logs
show that the state is being received.

`connect` method is changed to use `connection != null` check, the same
way it's done in [JS version][3].

[1]: https://github.com/phoenixframework/phoenix/blob/93db5ff3adf4c91a1ff1996e819e7dd5dfbddf1a/assets/js/phoenix.js#L906
[2]: https://github.com/davidstump/SwiftPhoenixClient/blob/ade5c27051a96aeeedff1594cb3e176b57a02f96/Sources/Socket.swift#L180
[3]: https://github.com/phoenixframework/phoenix/blob/93db5ff3adf4c91a1ff1996e819e7dd5dfbddf1a/assets/js/phoenix.js#L782
[4]: https://github.com/dsrees/JavaPhoenixClient/blob/master/src/main/kotlin/org/phoenixframework/PhxChannel.kt#L74
---
 .../kotlin/org/phoenixframework/PhxSocket.kt  | 10 +++--
 .../org/phoenixframework/PhxSocketTest.kt     | 42 +++++++++++++++++++
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/src/main/kotlin/org/phoenixframework/PhxSocket.kt b/src/main/kotlin/org/phoenixframework/PhxSocket.kt
index 119f81b..437ea67 100644
--- a/src/main/kotlin/org/phoenixframework/PhxSocket.kt
+++ b/src/main/kotlin/org/phoenixframework/PhxSocket.kt
@@ -99,7 +99,6 @@ open class PhxSocket(
     /// Timer to use when attempting to reconnect
     private var reconnectTimer: PhxTimer? = null
 
-
     private val gson: Gson = GsonBuilder()
             .setLenient()
             .setFieldNamingPolicy(FieldNamingPolicy.LOWER_CASE_WITH_UNDERSCORES)
@@ -154,8 +153,8 @@ open class PhxSocket(
     // Public
     //------------------------------------------------------------------------------
     /** True if the Socket is currently connected */
-    val isConnected: Boolean
-        get() = connection != null
+    var isConnected: Boolean = false
+        private set
 
     /**
      * Disconnects the Socket
@@ -173,7 +172,7 @@ open class PhxSocket(
      */
     fun connect() {
         // Do not attempt to reconnect if already connected
-        if (isConnected) return
+        if (connection != null) return
         connection = client.newWebSocket(request, this)
     }
 
@@ -440,6 +439,7 @@ open class PhxSocket(
     // WebSocketListener
     //------------------------------------------------------------------------------
     override fun onOpen(webSocket: WebSocket?, response: Response?) {
+        isConnected = true
         this.onConnectionOpened()
 
     }
@@ -451,10 +451,12 @@ open class PhxSocket(
     }
 
     override fun onClosed(webSocket: WebSocket?, code: Int, reason: String?) {
+        isConnected = false
         this.onConnectionClosed(code)
     }
 
     override fun onFailure(webSocket: WebSocket?, t: Throwable, response: Response?) {
+        isConnected = false
         this.onConnectionError(t, response)
     }
 }
diff --git a/src/test/kotlin/org/phoenixframework/PhxSocketTest.kt b/src/test/kotlin/org/phoenixframework/PhxSocketTest.kt
index 317c8ef..851f3b7 100644
--- a/src/test/kotlin/org/phoenixframework/PhxSocketTest.kt
+++ b/src/test/kotlin/org/phoenixframework/PhxSocketTest.kt
@@ -1,7 +1,9 @@
 package org.phoenixframework
 
 import com.google.common.truth.Truth.assertThat
+import okhttp3.WebSocket
 import org.junit.Test
+import org.mockito.Mockito
 
 class PhxSocketTest {
 
@@ -36,4 +38,44 @@ class PhxSocketTest {
         assertThat(PhxSocket("wss://localhost:4000/socket/websocket", spacesParams).endpoint.toString())
                 .isEqualTo("https://localhost:4000/socket/websocket?user_id=1&token=abc%20123")
     }
+
+    @Test
+    fun isConnected_isTrue_WhenSocketConnected() {
+        val mockSocket = Mockito.mock(WebSocket::class.java)
+        val socket = PhxSocket("http://localhost:4000/socket/websocket")
+
+        socket.onOpen(mockSocket, null)
+
+        assertThat(socket.isConnected).isTrue()
+    }
+
+    @Test
+    fun isConnected_isFalse_WhenSocketNotYetConnected() {
+        val socket = PhxSocket("http://localhost:4000/socket/websocket")
+
+
+        assertThat(socket.isConnected).isFalse()
+    }
+
+    @Test
+    fun isConnected_isFalse_WhenSocketFailed() {
+        val mockSocket = Mockito.mock(WebSocket::class.java)
+        val socket = PhxSocket("http://localhost:4000/socket/websocket")
+
+        socket.onFailure(mockSocket, RuntimeException(), null)
+
+
+        assertThat(socket.isConnected).isFalse()
+    }
+
+    @Test
+    fun isConnected_isFalse_WhenSocketClosed() {
+        val mockSocket = Mockito.mock(WebSocket::class.java)
+        val socket = PhxSocket("http://localhost:4000/socket/websocket")
+
+        socket.onClosed(mockSocket, 0, "closed")
+
+
+        assertThat(socket.isConnected).isFalse()
+    }
 }