Don't treat code 1000 as special

assets/js/phoenix/socket.js

@@ -150,7 +150,7 @@ export default class Socket {
     let awaitingConnectionOnPageShow = null
     if(phxWindow && phxWindow.addEventListener){
       phxWindow.addEventListener("pagehide", _e => {
-        // an ended connection only finishes closing, e.g. after disconnect or a normal close by the server
+        // an ended connection only finishes closing, e.g. after disconnect
         if(this.connection && !this.connection.ended){
           this.disconnect()
           awaitingConnectionOnPageShow = this.connectClock
@@ -582,7 +582,7 @@ export default class Socket {
     const connectClock = this.connectClock
     this.teardown(() => {
       // the socket was disconnected or connected again in the meantime, which the connection does not
-      // tell once an overlapping teardown gave up on it, or the server closed the connection normally
+      // tell once an overlapping teardown gave up on it
       if(connectClock !== this.connectClock || (connection && connection.ended)){ return }
       reconnect()
     })
@@ -653,24 +653,16 @@ export default class Socket {
 
   onConnClose(event){
     const connection = this.connection
-    let closeCode = event && event.code
     if(this.hasLogger()) this.log("transport", "close", event)
     this.clearHeartbeats()
     // we only close a connection to disconnect or to reconnect, which the code closing it takes
-    // care of, so its close event only confirms our close, even when the server sends 1000
+    // care of, so its close event only confirms our close. Any other close is reconnected, whatever
+    // its code, as the server, proxies and load balancers also close with 1000 (normal closure)
     const closedByUs = !!(connection && connection.closing)
-    // the server closes with 1000 when we should not reconnect, which also applies to
-    // reconnecting on visibility change
-    if(closeCode === 1000 && !closedByUs){
-      this.closeWasClean = true
-      if(connection){ connection.ended = true }
-      this.cancelFallback()
-      this.reconnectTimer.reset()
-    }
     // channel callbacks may disconnect or create a new connection, so finish cleaning up
     // this connection first and do not schedule a reconnect for its replacement
     this.triggerChanError("connection_closed", connection)
-    if(this.stillUses(connection) && !closedByUs && !this.closeWasClean && closeCode !== 1000){
+    if(this.stillUses(connection) && !closedByUs && !this.closeWasClean){
       this.reconnectTimer.scheduleTimeout()
     }
     this.stateChangeCallbacks.close.forEach(([, callback]) => callback(event))

assets/test/socket_test.js

@@ -220,22 +220,6 @@ describe("with transports", function (){
           expect(replaceSpy).toHaveBeenCalledTimes(1)
         })
 
-        it("does not fall back after a normal close", function (){
-          socket.connect()
-          connections[0].open()
-          // a normal close also cancels a reconnect that was already scheduled
-          socket.reconnectTimer.scheduleTimeout()
-
-          connections[0].finishClose(1000)
-          jest.advanceTimersByTime(5000)
-
-          expect(socket.transport).toBe(StubWebSocket)
-          expect(connections.length).toBe(1)
-          expect(socket.closeWasClean).toBe(true)
-          expect(socket.stateChangeCallbacks.open.length).toBe(0)
-          expect(socket.stateChangeCallbacks.error.length).toBe(0)
-        })
-
         it("remembers LongPoll when a later fallback connection opens", function (){
           socket.connect()
           jest.advanceTimersByTime(2500)
@@ -528,14 +512,15 @@ describe("with transports", function (){
       expect(socket.conn).toBe(connections[1])
     })
 
-    it("does not reconnect after the server closed the connection normally", function (){
+    it("reconnects when the page is shown again after the server closed the connection", function (){
       connections[0].finishClose(1000)
 
       listeners.pagehide()
       listeners.pageshow()
       jest.advanceTimersByTime(5000)
 
-      expect(connections.length).toBe(1)
+      expect(connections.length).toBe(2)
+      expect(socket.conn).toBe(connections[1])
     })
 
     it("does not reconnect after a disconnect that is still closing the connection", function (){
@@ -1895,17 +1880,9 @@ describe("with transports", function (){
       socket.connect()
     })
 
-    it("does not schedule reconnectTimer if normal close", function (){
-      const scheduleSpy = jest.spyOn(socket.reconnectTimer, "scheduleTimeout")
-      const event = {code: 1000}
-      socket.onConnClose(event)
-      expect(scheduleSpy).not.toHaveBeenCalled()
-    })
-
-    it("schedules reconnectTimer timeout if abnormal close", function (){
+    it.each([1000, 1001, 1006])("schedules reconnectTimer timeout if the server closes the connection (close: %s)", function (code){
       const scheduleSpy = jest.spyOn(socket.reconnectTimer, "scheduleTimeout")
-      const event = {code: 1006}
-      socket.onConnClose(event)
+      socket.onConnClose({code})
       expect(scheduleSpy).toHaveBeenCalledTimes(1)
     })
 
@@ -1915,13 +1892,6 @@ describe("with transports", function (){
       expect(scheduleSpy).not.toHaveBeenCalled()
     })
 
-    it("schedules reconnectTimer timeout if not normal close", function (){
-      const scheduleSpy = jest.spyOn(socket.reconnectTimer, "scheduleTimeout")
-      const event = {code: 1001}
-      socket.onConnClose(event)
-      expect(scheduleSpy).toHaveBeenCalledTimes(1)
-    })
-
     it("schedules reconnectTimer timeout if connection cannot be made after a previous clean disconnect", function (done){
       const scheduleSpy = jest.spyOn(socket.reconnectTimer, "scheduleTimeout")
       socket.disconnect(() => {
@@ -2029,16 +1999,17 @@ describe("with transports", function (){
         expect(second.state).toBe("joined")
       })
 
-      it("does not reconnect on visibility change after a normal close", function (){
+      it("reconnects on visibility change after the server closed the connection", function (){
         connections[0].finishClose(1000)
 
         socket.handleVisibilityChange()
         jest.advanceTimersByTime(5000)
 
-        expect(connections.length).toBe(1)
+        expect(connections.length).toBe(2)
+        expect(socket.conn).toBe(connections[1])
       })
 
-      it("still reconnects on visibility change when the server only confirmed our close", function (){
+      it("reconnects on visibility change when the server only confirmed our close", function (){
         // the heartbeat times out, so we close the connection with 1000, which the server confirms
         jest.advanceTimersByTime(2 * socket.heartbeatIntervalMs)
         connections[0].finishClose(1000)
@@ -2053,26 +2024,6 @@ describe("with transports", function (){
         expect(connections.length).toBe(2)
       })
 
-      it("does not let a scheduled reconnect replace a connection created after a normal close", function (){
-        socket.reconnectTimer.scheduleTimeout()
-        connections[0].finishClose(1000)
-
-        socket.connect()
-        jest.advanceTimersByTime(5000)
-
-        expect(connections.length).toBe(2)
-        expect(socket.conn).toBe(connections[1])
-      })
-
-      it("connects again when asked to after a normal close", function (){
-        connections[0].finishClose(1000)
-
-        socket.connect()
-
-        expect(connections.length).toBe(2)
-        expect(socket.conn).toBe(connections[1])
-      })
-
       it.each([1000, 1006])("preserves a connection opened by a channel error callback (close: %s)", function (code){
         const channel = socket.channel("topic")
         channel.join().trigger("ok", {})
@@ -2509,19 +2460,22 @@ describe("with transports", function (){
         expect(onMessage).not.toHaveBeenCalled()
       })
 
-      it("does not reconnect when a heartbeat reply is decoded after the server closed the connection", function (){
+      it("does not restart the heartbeats when a heartbeat reply is decoded after the server closed the connection", function (){
         jest.advanceTimersByTime(socket.heartbeatIntervalMs)
         encodings[0]()
         const [[, heartbeatRef]] = connections[0].sent
         const reply = {ref: heartbeatRef, topic: "phoenix", event: "phx_reply", payload: {status: "ok", response: {}}}
         connections[0].onmessage({data: encode(reply)})
+        // the teardown before reconnecting stops the heartbeats, so we reconnect after they would have run
+        socket.reconnectTimer.timerCalc = () => 3 * socket.heartbeatIntervalMs
         connections[0].finishClose(1000)
+        const heartbeatSpy = jest.spyOn(socket, "sendHeartbeat")
 
         decodings[0]()
-        jest.advanceTimersByTime(3 * socket.heartbeatIntervalMs)
+        jest.advanceTimersByTime(2 * socket.heartbeatIntervalMs)
 
+        expect(heartbeatSpy).not.toHaveBeenCalled()
         expect(connections.length).toBe(1)
-        expect(socket.closeWasClean).toBe(true)
       })
     })
 

lib/phoenix/socket.ex

@@ -569,11 +569,7 @@ defmodule Phoenix.Socket do
   end
 
   def __info__(%Broadcast{event: "disconnect"}, state) do
-    # Close code 1001 ("Going Away") signals the client that the connection
-    # is intentionally closed but a reconnect is expected — phoenix.js gates
-    # reconnects behind a closeCode !== 1000 check.
-    # See https://github.com/mtrudel/bandit/issues/582.
-    {:stop, {:shutdown, :disconnected}, 1001, state}
+    {:stop, {:shutdown, :disconnected}, state}
   end
 
   def __info__(:socket_drain, state) do

lib/phoenix/transports/long_poll_server.ex

@@ -117,10 +117,6 @@ defmodule Phoenix.Transports.LongPoll.Server do
       {:stop, reason, handler_state} ->
         state = %{state | handler: {handler, handler_state}}
         {:stop, reason, state}
-
-      {:stop, reason, _code, handler_state} ->
-        state = %{state | handler: {handler, handler_state}}
-        {:stop, reason, state}
     end
   end
 

test/phoenix/integration/websocket_channels_test.exs

@@ -868,11 +868,19 @@ defmodule Phoenix.Integration.WebSocketChannelsTest do
 
           assert_receive {:DOWN, _, :process, ^sock, :normal}
           assert_receive {:DOWN, _, :process, ^chan1, shutdown}
-          # :shutdown for cowboy, {:shutdown, :closed} for cowboy 2, {:shutdown, :disconnected}
-          # for bandit
-          assert shutdown in [:shutdown, {:shutdown, :closed}, {:shutdown, :disconnected}]
+          # :shutdown for cowboy, {:shutdown, :closed} for cowboy 2. Bandit closes normally and
+          # waits for the client, so it is {:shutdown, :local_closed} or {:shutdown, :peer_closed}
+          # depending on whether it sees the client's close frame or the closed connection first
+          shutdowns = [
+            :shutdown,
+            {:shutdown, :closed},
+            {:shutdown, :local_closed},
+            {:shutdown, :peer_closed}
+          ]
+
+          assert shutdown in shutdowns
           assert_receive {:DOWN, _, :process, ^chan2, shutdown}
-          assert shutdown in [:shutdown, {:shutdown, :closed}, {:shutdown, :disconnected}]
+          assert shutdown in shutdowns
         end
 
         test "duplicate join event closes existing channel" do

test/phoenix/socket/socket_test.exs

@@ -137,20 +137,4 @@ defmodule Phoenix.SocketTest do
       assert DrainerSpecSocket.drainer_spec(drainer: false, endpoint: Endpoint) == :ignore
     end
   end
-
-  describe "__info__/2" do
-    alias Phoenix.Socket.Broadcast
-
-    test "disconnect broadcast emits close code 1001 so phoenix.js reconnects" do
-      # phoenix.js gates reconnects on `closeCode !== 1000`.
-      # Servers might interpret `{:shutdown, :disconnected}`
-      # as code 1000, so we pass 1001 explicitly to force a retry.
-      # See https://github.com/mtrudel/bandit/issues/582.
-      state = make_ref()
-      msg = %Broadcast{topic: "t", event: "disconnect", payload: %{}}
-
-      assert {:stop, {:shutdown, :disconnected}, 1001, ^state} =
-               Phoenix.Socket.__info__(msg, state)
-    end
-  end
 end