Repository: phoenixframework/phoenix
Pull request #3141 (2018-11-04 19:55:46)

Allow channels to immediately reconnect when the socket does
The timer state must be known in order to know if the channel was attempting to rejoin or not. It is
not correct to start a channel rejoin timer if it was not previously running. This involved changing the timer logic up a bit to clear out `this.timer` when it's done processing

Related to #3139 


Patch:
diff --git a/assets/js/phoenix.js b/assets/js/phoenix.js
index 9f56a052d..78149265a 100644
--- a/assets/js/phoenix.js
+++ b/assets/js/phoenix.js
@@ -838,6 +838,7 @@ export class Socket {
     this.flushSendBuffer()
     this.reconnectTimer.reset()
     this.resetHeartbeat()
+    this.resetChannelTimers()
     this.stateChangeCallbacks.open.forEach( callback => callback() )
   }
 
@@ -986,6 +987,15 @@ export class Socket {
       }
     })
   }
+
+  /**
+   * @private
+   */
+  resetChannelTimers() {
+    this.channels.forEach(channel => {
+      channel.rejoinTimer.restart()
+    })
+  }
 }
 
 
@@ -1346,18 +1356,31 @@ class Timer {
 
   reset(){
     this.tries = 0
-    clearTimeout(this.timer)
+    this.clearTimer()
+  }
+
+  restart(){
+    const processing = this.timer !== null
+    this.reset()
+    if (processing){
+      this.scheduleTimeout()
+    }
   }
 
   /**
    * Cancels any previous scheduleTimeout and schedules callback
    */
   scheduleTimeout(){
-    clearTimeout(this.timer)
+    this.clearTimer()
 
     this.timer = setTimeout(() => {
       this.tries = this.tries + 1
       this.callback()
     }, this.timerCalc(this.tries + 1))
   }
+
+  clearTimer() {
+    clearTimeout(this.timer)
+    this.timer = null
+  }
 }
diff --git a/installer/lib/mix/tasks/phx.new.ex b/installer/lib/mix/tasks/phx.new.ex
index a69b202e2..1eca2415e 100644
--- a/installer/lib/mix/tasks/phx.new.ex
+++ b/installer/lib/mix/tasks/phx.new.ex
@@ -136,25 +136,25 @@ defmodule Mix.Tasks.Phx.New do
   defp prompt_to_install_deps(%Project{} = project, generator) do
     install? = Mix.shell.yes?("\nFetch and install dependencies?")
 
+    cd_step = ["$ cd #{relative_app_path(project.project_path)}"]
+
     maybe_cd(project.project_path, fn ->
-      mix_pending =
-        install_mix(install?)
+      mix_step = install_mix(install?)
 
       compile =
-        case mix_pending do
+        case mix_step do
           [] -> Task.async(fn -> rebar_available?() && cmd("mix deps.compile") end)
           _  -> Task.async(fn -> :ok end)
         end
 
-      webpack_pending = install_webpack(install?, project)
+      webpack_step = install_webpack(install?, project)
       Task.await(compile, :infinity)
 
       if Project.webpack?(project) and !System.find_executable("npm") do
         print_webpack_info(project, generator)
       end
 
-      pending = mix_pending ++ (webpack_pending || [])
-      print_missing_commands(pending, project.project_path)
+      print_missing_steps(cd_step ++ mix_step ++ webpack_step)
 
       if Project.ecto?(project) do
         print_ecto_info(project, generator)
@@ -208,16 +208,7 @@ defmodule Mix.Tasks.Phx.New do
     """
   end
 
-  defp print_missing_commands([], path) do
-    Mix.shell.info """
-
-    We are all set! Go into your application by running:
-
-        $ cd #{relative_app_path(path)}
-    """
-  end
-  defp print_missing_commands(commands, path) do
-    steps = ["$ cd #{relative_app_path(path)}" | commands]
+  defp print_missing_steps(steps) do
     Mix.shell.info """
 
     We are almost there! The following steps are missing:
diff --git a/installer/templates/phx_single/formatter.exs b/installer/templates/phx_single/formatter.exs
index be3b79494..1aee095fc 100644
--- a/installer/templates/phx_single/formatter.exs
+++ b/installer/templates/phx_single/formatter.exs
@@ -1,5 +1,6 @@
 [
-  import_deps: [<%= if ecto do %>:ecto, <% end %>:phoenix],
-  inputs: ["*.{ex,exs}", "{config,lib,priv,test}/**/*.{ex,exs}"]<%= if ecto do %>,
-  subdirectories: ["priv/*/migrations"]<% end %>
+  import_deps: [<%= if ecto do %>:ecto, <% end %>:phoenix],<%= if ecto do %>
+  inputs: ["*.{ex,exs}", "priv/*/seeds.exs", "{config,lib,test}/**/*.{ex,exs}"],
+  subdirectories: ["priv/*/migrations"]<% else %>
+  inputs: ["*.{ex,exs}", "{config,lib,test}/**/*.{ex,exs}"]<% end %>
 ]
diff --git a/installer/templates/phx_umbrella/apps/app_name/formatter.exs b/installer/templates/phx_umbrella/apps/app_name/formatter.exs
index 12c5d8dfc..b599203bb 100644
--- a/installer/templates/phx_umbrella/apps/app_name/formatter.exs
+++ b/installer/templates/phx_umbrella/apps/app_name/formatter.exs
@@ -1,5 +1,6 @@
 [<%= if ecto do %>
   import_deps: [:ecto],
-  subdirectories: ["priv/*/migrations"],<% end %>
-  inputs: ["*.{ex,exs}", "{config,lib,test}/**/*.{ex,exs}"]
+  inputs: ["*.{ex,exs}", "priv/*/seeds.exs", "{config,lib,test}/**/*.{ex,exs}"],
+  subdirectories: ["priv/*/migrations"]<% else %>
+  inputs: ["*.{ex,exs}", "{config,lib,test}/**/*.{ex,exs}"]<% end %>
 ]
diff --git a/lib/phoenix/channel.ex b/lib/phoenix/channel.ex
index 6e528f1f5..312296134 100644
--- a/lib/phoenix/channel.ex
+++ b/lib/phoenix/channel.ex
@@ -20,12 +20,12 @@ defmodule Phoenix.Channel do
   matched in your channels' `join/3` callback to pluck out the scoped pattern:
 
       # handles the special `"lobby"` subtopic
-      def join("room:lobby", _auth_message, socket) do
+      def join("room:lobby", _payload, socket) do
         {:ok, socket}
       end
 
       # handles any other subtopic as the room ID, for example `"room:12"`, `"room:34"`
-      def join("room:" <> room_id, auth_message, socket) do
+      def join("room:" <> room_id, _payload, socket) do
         {:ok, socket}
       end
 
@@ -243,8 +243,8 @@ defmodule Phoenix.Channel do
   simply relay the `%Phoenix.Socket.Broadcast{}` event and payload:
 
       alias Phoenix.Socket.Broadcast
-      def handle_info(%Broadcast{topic: _, event: ev, payload: payload}, socket) do
-        push(socket, ev, payload)
+      def handle_info(%Broadcast{topic: _, event: event, payload: payload}, socket) do
+        push(socket, event, payload)
         {:noreply, socket}
       end
 
@@ -267,31 +267,74 @@ defmodule Phoenix.Channel do
   @type socket_ref :: {transport_pid :: Pid, serializer :: module,
                        topic :: binary, ref :: binary, join_ref :: binary}
 
-  @callback join(topic :: binary, auth_msg :: map, Socket.t) ::
+  @doc """
+  Handle channel joins by `topic`.
+
+  To authorize a socket, return `{:ok, socket}` or `{:ok, reply, socket}`. To
+  refuse authorization, return `{:error, reason}`.
+
+  ## Example
+
+      def join("room:lobby", payload, socket) do
+        if authorized?(payload) do
+          {:ok, socket}
+        else
+          {:error, %{reason: "unauthorized"}}
+        end
+      end
+
+  """
+  @callback join(topic :: binary, payload :: map, socket :: Socket.t) ::
               {:ok, Socket.t} |
-              {:ok, map, Socket.t} |
-              {:error, map}
+              {:ok, reply :: map, Socket.t} |
+              {:error, reason :: map}
+
+  @doc """
+  Handle incoming `event`s.
+
+  ## Example
 
-  @callback handle_in(event :: String.t, msg :: map, Socket.t) ::
+      def handle_in("ping", payload, socket) do
+        {:reply, {:ok, payload}, socket}
+      end
+
+  """
+  @callback handle_in(event :: String.t, payload :: map, socket :: Socket.t) ::
               {:noreply, Socket.t} |
               {:noreply, Socket.t, timeout | :hibernate} |
               {:reply, reply, Socket.t} |
               {:stop, reason :: term, Socket.t} |
               {:stop, reason :: term, reply, Socket.t}
 
-  @callback handle_out(event :: String.t, msg :: map, Socket.t) ::
+  @doc """
+  Intercepts outgoing `event`s.
+
+  See `intercept/1`.
+  """
+  @callback handle_out(event :: String.t, payload :: map, socket :: Socket.t) ::
               {:noreply, Socket.t} |
               {:noreply, Socket.t, timeout | :hibernate} |
               {:stop, reason :: term, Socket.t}
 
-  @callback handle_info(term, Socket.t) ::
+  @doc """
+  Handle regular Elixir process messages.
+
+  See `GenServer.handle_info/2`.
+  """
+  @callback handle_info(msg :: term, socket :: Socket.t) ::
               {:noreply, Socket.t} |
               {:stop, reason :: term, Socket.t}
 
+  @doc false
   @callback code_change(old_vsn, Socket.t, extra :: term) ::
               {:ok, Socket.t} |
               {:error, reason :: term} when old_vsn: term | {:down, term}
 
+  @doc """
+  Invoked when the channel process is about to exit.
+
+  See `GenServer.terminate/2`.
+  """
   @callback terminate(reason :: :normal | :shutdown | {:shutdown, :left | :closed | term}, Socket.t) ::
               term
 
@@ -348,6 +391,7 @@ defmodule Phoenix.Channel do
   `handle_out/3` callbacks must return one of:
 
       {:noreply, Socket.t} |
+      {:noreply, Socket.t, timeout | :hibernate} |
       {:stop, reason :: term, Socket.t}
 
   """
diff --git a/lib/phoenix/endpoint/cowboy2_handler.ex b/lib/phoenix/endpoint/cowboy2_handler.ex
index 3e091d73b..32a1c5490 100644
--- a/lib/phoenix/endpoint/cowboy2_handler.ex
+++ b/lib/phoenix/endpoint/cowboy2_handler.ex
@@ -25,7 +25,7 @@ defmodule Phoenix.Endpoint.Cowboy2Handler do
               {:cowboy_websocket, req, [handler | state], cowboy_opts}
 
             {:error, %{adapter: {@connection, req}}} ->
-              {:error, req}
+              {:ok, req, {handler, opts}}
           end
 
         {:plug, handler, opts} ->


Test patch:
diff --git a/assets/test/socket_test.js b/assets/test/socket_test.js
index 4790db775..fe38507b5 100644
--- a/assets/test/socket_test.js
+++ b/assets/test/socket_test.js
@@ -605,6 +605,25 @@ describe("onConnOpen", () => {
     assert.ok(spy.calledOnce)
   })
 
+  it("resets all channel timers and schedules a timeout if the timer was in progress", () => {
+    const channel = socket.channel("topic", {})
+    const channel2 = socket.channel("topic2", {})
+
+    channel.rejoinTimer.tries = 1
+    channel2.rejoinTimer.tries = 2
+    channel2.rejoinTimer.scheduleTimeout()
+
+    assert.equal(channel.rejoinTimer.timer, null)
+    assert.notEqual(channel2.rejoinTimer.timer, null)
+
+    socket.onConnOpen()
+
+    assert.equal(channel.rejoinTimer.tries, 0)
+    assert.equal(channel2.rejoinTimer.tries, 0)
+    assert.equal(channel.rejoinTimer.timer, null)
+    assert.notEqual(channel2.rejoinTimer.timer, null)
+  })
+
   it("triggers onOpen callback", () => {
     const spy = sinon.spy()
 
diff --git a/installer/test/phx_new_test.exs b/installer/test/phx_new_test.exs
index bc8b9beb1..680f41c1d 100644
--- a/installer/test/phx_new_test.exs
+++ b/installer/test/phx_new_test.exs
@@ -24,7 +24,13 @@ defmodule Mix.Tasks.Phx.NewTest do
       Mix.Tasks.Phx.New.run([@app_name])
 
       assert_file "phx_blog/README.md"
-      assert_file "phx_blog/.formatter.exs", ~r/import_deps: \[:ecto, :phoenix\]/
+
+      assert_file "phx_blog/.formatter.exs", fn file ->
+        assert file =~ "import_deps: [:ecto, :phoenix]"
+        assert file =~ "inputs: [\"*.{ex,exs}\", \"priv/*/seeds.exs\", \"{config,lib,test}/**/*.{ex,exs}\"]"
+        assert file =~ "subdirectories: [\"priv/*/migrations\"]"
+      end
+
       assert_file "phx_blog/mix.exs", fn file ->
         assert file =~ "app: :phx_blog"
         refute file =~ "deps_path: \"../../deps\""
@@ -173,6 +179,12 @@ defmodule Mix.Tasks.Phx.NewTest do
       config = ~r/config :phx_blog, PhxBlog.Repo,/
       refute File.exists?("phx_blog/lib/phx_blog/repo.ex")
 
+      assert_file "phx_blog/.formatter.exs", fn file ->
+        assert file =~ "import_deps: [:phoenix]"
+        assert file =~ "inputs: [\"*.{ex,exs}\", \"{config,lib,test}/**/*.{ex,exs}\"]"
+        refute file =~ "subdirectories:"
+      end
+
       assert_file "phx_blog/mix.exs", &refute(&1 =~ ~r":phoenix_ecto")
 
       assert_file "phx_blog/config/config.exs", fn file ->
diff --git a/installer/test/phx_new_umbrella_test.exs b/installer/test/phx_new_umbrella_test.exs
index 269f9dde8..99dba6b6f 100644
--- a/installer/test/phx_new_umbrella_test.exs
+++ b/installer/test/phx_new_umbrella_test.exs
@@ -76,6 +76,18 @@ defmodule Mix.Tasks.Phx.New.UmbrellaTest do
         assert file =~ ":inet6"
       end
 
+      assert_file app_path(@app, ".formatter.exs"), fn file ->
+        assert file =~ "import_deps: [:ecto]"
+        assert file =~ "inputs: [\"*.{ex,exs}\", \"priv/*/seeds.exs\", \"{config,lib,test}/**/*.{ex,exs}\"]"
+        assert file =~ "subdirectories: [\"priv/*/migrations\"]"
+      end
+
+      assert_file web_path(@app, ".formatter.exs"), fn file ->
+        assert file =~ "inputs: [\"*.{ex,exs}\", \"{config,lib,test}/**/*.{ex,exs}\"]"
+        refute file =~ "import_deps: [:ecto]"
+        refute file =~ "subdirectories:"
+      end
+
       assert_file app_path(@app, "lib/#{@app}/application.ex"), ~r/defmodule PhxUmb.Application do/
       assert_file app_path(@app, "lib/#{@app}/application.ex"), ~r/PhxUmb.Repo/
       assert_file app_path(@app, "lib/#{@app}.ex"), ~r/defmodule PhxUmb do/
diff --git a/lib/phoenix/test/conn_test.ex b/lib/phoenix/test/conn_test.ex
index 2cce7bf99..416030031 100644
--- a/lib/phoenix/test/conn_test.ex
+++ b/lib/phoenix/test/conn_test.ex
@@ -434,7 +434,7 @@ defmodule Phoenix.ConnTest do
       assert redirected_to(conn, 301) =~ "/foo/bar"
       assert redirected_to(conn, :moved_permanently) =~ "/foo/bar"
   """
-  @spec redirected_to(Conn.t, status :: non_neg_integer) :: Conn.t
+  @spec redirected_to(Conn.t, status :: non_neg_integer) :: String.t
   def redirected_to(conn, status \\ 302)
 
   def redirected_to(%Conn{state: :unset}, _status) do
diff --git a/priv/templates/phx.gen.context/test_cases.exs b/priv/templates/phx.gen.context/test_cases.exs
index e83b1252f..92f73323f 100644
--- a/priv/templates/phx.gen.context/test_cases.exs
+++ b/priv/templates/phx.gen.context/test_cases.exs
@@ -36,9 +36,7 @@
 
     test "update_<%= schema.singular %>/2 with valid data updates the <%= schema.singular %>" do
       <%= schema.singular %> = <%= schema.singular %>_fixture()
-      assert {:ok, %<%= inspect schema.alias %>{} = <%= schema.singular %>} = <%= inspect context.alias %>.update_<%= schema.singular %>(<%= schema.singular %>, @update_attrs)
-
-      <%= for {field, value} <- schema.params.update do %>
+      assert {:ok, %<%= inspect schema.alias %>{} = <%= schema.singular %>} = <%= inspect context.alias %>.update_<%= schema.singular %>(<%= schema.singular %>, @update_attrs)<%= for {field, value} <- schema.params.update do %>
       assert <%= schema.singular %>.<%= field %> == <%= Mix.Phoenix.Schema.value(schema, field, value) %><% end %>
     end