Updated transport to call onClose after onError

Author: Daniel Rees

src/main/kotlin/org/phoenixframework/Socket.kt

@@ -54,9 +54,6 @@ const val WS_CLOSE_NORMAL = 1000
 /** RFC 6455: indicates that the connection was closed abnormally */
 const val WS_CLOSE_ABNORMAL = 1006
 
-/** The socket was closed due to a SocketException. Likely the client lost connectivity */
-// DEPRECATED
-const val WS_CLOSE_SOCKET_EXCEPTION = 4000
 
 /**
  * Connects to a Phoenix Server
@@ -414,8 +411,14 @@ class Socket(
         ref = pendingHeartbeatRef)
   }
 
-  internal fun abnormalClose(reason: String) {
+  private fun abnormalClose(reason: String) {
     this.closeWasClean = false
+
+    /*
+      We use NORMAL here since the client is the one determining to close the connection. However,
+      we keep a flag `closeWasClean` set to false so that the client knows that it should attempt
+      to reconnect.
+     */
     this.connection?.disconnect(WS_CLOSE_NORMAL, reason)
   }
 

src/main/kotlin/org/phoenixframework/Transport.kt

@@ -27,7 +27,6 @@ import okhttp3.Request
 import okhttp3.Response
 import okhttp3.WebSocket
 import okhttp3.WebSocketListener
-import java.io.IOException
 import java.net.URL
 
 /**
@@ -130,13 +129,28 @@ class WebSocketTransport(
   }
 
   override fun onFailure(webSocket: WebSocket, t: Throwable, response: Response?) {
+    // Set the state of the Transport as CLOSED since no more data will be received
     this.readyState = Transport.ReadyState.CLOSED
+
+    // Invoke the onError callback, to inform of the error
     this.onError?.invoke(t, response)
-    
-    // Check if the socket was closed for some recoverable reason
-    when (t) {
-      is IOException -> this.onClosed(webSocket, WS_CLOSE_SOCKET_EXCEPTION, "IOException")
-    }
+
+    /*
+      According to the OkHttp documentation, `onFailure` will be
+
+      "Invoked when a web socket has been closed due to an error reading from or writing to the
+      network. Both outgoing and incoming messages may have been lost. No further calls to this
+      listener will be made."
+
+      This means `onClose` will never be called which will never kick off the socket reconnect
+      attempts.
+
+      The JS WebSocket class calls `onError` and then `onClose` which will then trigger
+      the reconnect logic inside of the PhoenixClient. In order to mimic this behavior and abstract
+      this detail of OkHttp away from the PhoenixClient, the `WebSocketTransport` class should
+      convert `onFailure` calls to an `onError` and `onClose` sequence.
+     */
+    this.onClose?.invoke(WS_CLOSE_ABNORMAL)
   }
 
   override fun onClosing(webSocket: WebSocket, code: Int, reason: String) {

src/test/kotlin/org/phoenixframework/SocketTest.kt

@@ -294,6 +294,14 @@ class SocketTest {
       verify(connection).disconnect(WS_CLOSE_NORMAL)
     }
 
+    @Test
+    internal fun `flags the socket as closed cleanly`() {
+      assertThat(socket.closeWasClean).isFalse()
+
+      socket.disconnect()
+      assertThat(socket.closeWasClean).isTrue()
+    }
+
     @Test
     internal fun `calls callback`() {
       val mockCallback = mock<() -> Unit> {}

src/test/kotlin/org/phoenixframework/WebSocketTransportTest.kt

@@ -14,8 +14,6 @@ import org.junit.jupiter.api.Nested
 import org.junit.jupiter.api.Test
 import org.mockito.Mock
 import org.mockito.MockitoAnnotations
-import org.phoenixframework.Transport
-import org.phoenixframework.WebSocketTransport
 import java.net.SocketException
 import java.net.URL
 
@@ -124,7 +122,7 @@ class WebSocketTransportTest {
       val throwable = SocketException()
       transport.onFailure(mockWebSocket, throwable, mockResponse)
       verify(mockOnError).invoke(throwable, mockResponse)
-      verify(mockOnClose).invoke(4000)
+      verify(mockOnClose).invoke(1006)
     }
 
     /* End OnFailure */