Stop upload channel with {:shutdown, :closed} on writer error so the LiveView survives (#4320)

A custom UploadWriter returning {:error, _} (from write_chunk/2 or
close/2) currently brings down the whole LiveView, not just the failed
entry. The upload channel replies with the error but stays alive. The JS
client's error/1 then calls leave(), so the channel exits
{:shutdown, :left}, and the parent LiveView's {:DOWN} handler stops on
any non-:closed upload-channel exit ({:channel_upload_exit, _}).

Oversized files already self-close with {:shutdown, :closed} in the same
handle_in/3. Writer errors were the one branch that didn't. Stopping with
{:shutdown, :closed} drops the failed entry (threading the stored cid so
component uploads drop too) and the LiveView survives. The error reply is
still delivered to the client before the channel stops.

A close(:done) failure already closed the writer before this branch runs,
so the branch now skips a second close/2 (as handle_call(:cancel, ...)
already does). Otherwise a writer that is not safe to close twice could
raise and crash the channel with an abnormal exit, killing the LiveView
through the same path this change is meant to fix.

Author: aptinio

lib/phoenix_live_view/upload_channel.ex

@@ -102,15 +102,20 @@ defmodule Phoenix.LiveView.UploadChannel do
           {:reply, :ok, new_socket}
 
         {:error, reason, new_socket} ->
+          # a close(:done) failure already closed the writer; don't close it twice
           new_socket =
-            case close_writer(new_socket, {:error, reason}) do
-              {:ok, new_socket} -> new_socket
-              {:error, _reason, new_socket} -> new_socket
+            if new_socket.assigns.writer_closed? do
+              new_socket
+            else
+              case close_writer(new_socket, {:error, reason}) do
+                {:ok, new_socket} -> new_socket
+                {:error, _reason, new_socket} -> new_socket
+              end
             end
 
           Channel.report_writer_error(socket.assigns.live_view_pid, reason)
 
-          {:reply, {:error, %{reason: :writer_error}}, new_socket}
+          {:stop, {:shutdown, :closed}, {:error, %{reason: :writer_error}}, new_socket}
       end
     else
       reply = %{reason: :file_size_limit_exceeded, limit: max_file_size}

test/phoenix_live_view/upload/channel_test.exs

@@ -42,10 +42,35 @@ defmodule Phoenix.LiveView.UploadChannelTest do
     end
   end
 
+  defmodule CloseErrorWriter do
+    @behaviour Phoenix.LiveView.UploadWriter
+
+    @impl true
+    defdelegate init(test_name), to: TestWriter
+
+    @impl true
+    defdelegate meta(test_name), to: TestWriter
+
+    @impl true
+    defdelegate write_chunk(data, test_name), to: TestWriter
+
+    @impl true
+    def close(test_name, :done) do
+      send(test_name, {:close, :done})
+      {:error, :close_failed}
+    end
+
+    def close(test_name, reason), do: TestWriter.close(test_name, reason)
+  end
+
   def build_writer(_name, %Phoenix.LiveView.UploadEntry{}, %Phoenix.LiveView.Socket{}) do
     {TestWriter, :test_writer}
   end
 
+  def build_close_error_writer(_name, %Phoenix.LiveView.UploadEntry{}, %Phoenix.LiveView.Socket{}) do
+    {CloseErrorWriter, :test_writer}
+  end
+
   def valid_token(lv_pid, ref) do
     LiveView.Static.sign_token(@endpoint, %{pid: lv_pid, ref: ref})
   end
@@ -858,24 +883,71 @@ defmodule Phoenix.LiveView.UploadChannelTest do
 
       @tag allow: [
              max_entries: 1,
-             chunk_size: 50,
+             chunk_size: 5,
              accept: :any,
              writer: &__MODULE__.build_writer/3
            ]
-      test "writer with error", %{lv: lv} do
+      test "writer write_chunk/2 error self-closes the upload channel without bringing down the LiveView",
+           %{lv: lv} do
         Process.register(self(), :test_writer)
 
-        content = "error"
+        # first chunk writes ok; second chunk ("error") fails write_chunk
+        avatar =
+          file_input(lv, "form", :avatar, [
+            %{name: "foo.jpeg", content: "00000error"}
+          ])
+
+        assert render_upload(avatar, "foo.jpeg", 50) =~ "#{@context}:foo.jpeg:50%"
+        assert %{"foo.jpeg" => channel_pid} = UploadClient.channel_pids(avatar)
+
+        unlink(channel_pid, lv, avatar)
+        Process.monitor(channel_pid)
+
+        render_upload(avatar, "foo.jpeg", 50)
+        assert_receive {:close, {:error, :custom_error}}
+
+        # the upload channel self-closes instead of being left {:shutdown, :left}
+        assert_receive {:DOWN, _ref, :process, ^channel_pid, {:shutdown, :closed}}, 1000
+
+        # the failed entry is dropped and the LiveView is still alive
+        assert get_uploaded_entries(lv, :avatar) == {[], []}
+      end
+
+      @tag allow: [
+             max_entries: 1,
+             chunk_size: 50,
+             accept: :any,
+             writer: &__MODULE__.build_close_error_writer/3
+           ]
+      test "writer close error self-closes the upload channel without bringing down the LiveView",
+           %{lv: lv} do
+        Process.register(self(), :test_writer)
+
+        content = String.duplicate("0", 100)
 
         avatar =
           file_input(lv, "form", :avatar, [
             %{name: "foo.jpeg", content: content}
           ])
 
-        assert render_upload(avatar, "foo.jpeg") =~
-                 ~s/entry_error:{:writer_failure, :custom_error}/
+        assert render_upload(avatar, "foo.jpeg", 50) =~ "#{@context}:foo.jpeg:50%"
+        assert %{"foo.jpeg" => channel_pid} = UploadClient.channel_pids(avatar)
 
-        assert_receive {:close, {:error, :custom_error}}
+        unlink(channel_pid, lv, avatar)
+        Process.monitor(channel_pid)
+
+        # uploading the final chunk completes the file and runs close(:done), which errors
+        render_upload(avatar, "foo.jpeg", 50)
+        assert_receive {:close, :done}
+
+        # close(:done) already closed the writer, so the error branch must not close it again
+        refute_receive {:close, {:error, :close_failed}}
+
+        # the upload channel self-closes instead of being left {:shutdown, :left}
+        assert_receive {:DOWN, _ref, :process, ^channel_pid, {:shutdown, :closed}}, 1000
+
+        # the failed entry is dropped and the LiveView is still alive
+        assert get_uploaded_entries(lv, :avatar) == {[], []}
       end
     end
   end