Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix race conditions when upload progress redirects #3676

Merged
merged 1 commit into from
Feb 14, 2025

Conversation

SteffenDE
Copy link
Collaborator

@SteffenDE SteffenDE commented Feb 13, 2025

When someone does a push_navigate in the progress callback of an upload, the render_upload function of LiveViewTest sometimes did not properly return the live_redirect error tuple. This happened because the UploadChannel closes as soon as an upload is consumed. When this happens in the progress callback, the test code and the channel code raced:
The channel sends a redirect message to itself (or the root view) which shuts down once it receives it. The test code triggers a new render through the ClientProxy. Now it could happen that:

  1. the test process can successfully call the ClientProxy, but the ClientProxy then crashed while pinging the channel
  2. or the ClientProxy succeeds in pinging and actually returns the new HTML while the test expects the redirect error tuple
  3. or the channel and client proxy shut down in time for the test process to fail rendering and return the error tuple as expected

We fix this race by ensuring that we ping the view (and root view) as soon as the upload is finished. This ensures that the channel has time to send the redirect message before the render call can ping it again. Thus, we always get the expected shutdown at the latest when the render call pings the channel.

Fixes #3662.
Relates to: #2047
Relates to: #1620

@SteffenDE SteffenDE force-pushed the sd-upload-progress-redirect branch from 75d0588 to fcd3e69 Compare February 13, 2025 14:37
Comment on lines 1074 to 1077
case render_tree(view_or_element) do
{:error, reason} -> {:error, reason}
tree -> DOM.to_html(tree)
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this specifically prevents the error reported in the issue, as render_tree() can also return {:error, ...}

  5) test attempt: 1 redirects to the success page after a successful upload (DemoWeb.UploadLive.ShowTest)
     test/demo_web/live/upload_live/show_test.exs:8
     ** (FunctionClauseError) no function clause matching in Floki.RawHTML.build_raw_html/6

     The following arguments were given to Floki.RawHTML.build_raw_html/6:

         # 1
         [error: {:live_redirect, %{kind: :push, to: "/success"}}]

         # 2
         []

         # 3
         &Floki.Entities.encode/1

         # 4
         []

         # 5
         ["area", "base", "br", "col", "command", "embed", "hr", "img", "input", "keygen", "link", "menuitem", "meta", "param", "source", "track", "wbr"]

         # 6
         []

     Attempted function clauses (showing 6 out of 6):

         defp build_raw_html([], acc, _encoder, _pad, _self_closing_tags, _line_ending)
         defp build_raw_html([string | tail], acc, encoder, pad, self_closing_tags, line_ending) when is_binary(string)
         defp build_raw_html([{:comment, comment} | tail], acc, encoder, pad, self_closing_tags, line_ending)
         defp build_raw_html([{:pi, tag, attrs} | tail], acc, encoder, pad, self_closing_tags, line_ending)
         defp build_raw_html([{:doctype, type, public, system} | tail], acc, encoder, pad, self_closing_tags, line_ending)
         defp build_raw_html([{type, attrs, children} | tail], acc, encoder, pad, self_closing_tags, line_ending)

     code: html = render_upload(avatar, "kenya.jpg")
     stacktrace:
       (floki 0.37.0) lib/floki/raw_html.ex:73: Floki.RawHTML.build_raw_html/6
       (floki 0.37.0) lib/floki/raw_html.ex:68: Floki.RawHTML.raw_html/2
       (phoenix_live_view 1.0.4) lib/phoenix_live_view/test/live_view_test.ex:2024: Phoenix.LiveViewTest.render_chunk/3
       test/demo_web/live/upload_live/show_test.exs:22: (test)

@SteffenDE SteffenDE force-pushed the sd-upload-progress-redirect branch from fcd3e69 to 76c9c10 Compare February 14, 2025 16:05
When someone does a push_navigate in the progress callback of an upload,
the render_upload function of LiveViewTest sometimes did not properly
return the live_redirect error tuple. This happened because the Upload-
Channel closes as soon as an upload is consumed. When this happens in the
progress callback, the test code and the channel code raced:
The channel sends a redirect message to itself (or the root view) which
shuts down once it receives it. The test code triggers a new render
through the ClientProxy. Now it could happen that:

1. the test process can successfully call the ClientProxy, but the
   ClientProxy then crashed while pinging the channel
2. or the ClientProxy succeeds in pinging and actually returns the new
   HTML while the test expects the redirect error tuple
3. or the channel and client proxy shut down in time for the test process
   to fail rendering and return the error tuple as expected

We fix this race by ensuring that we ping the view (and root view) as
soon as the upload is finished. This ensures that the channel has time to
send the redirect message before the render call can ping it again. Thus,
we always get the expected shutdown at the latest when the render call
pings the channel.
@SteffenDE SteffenDE force-pushed the sd-upload-progress-redirect branch from 76c9c10 to cd8c015 Compare February 14, 2025 16:08
@SteffenDE SteffenDE marked this pull request as ready for review February 14, 2025 16:08
@SteffenDE SteffenDE merged commit d4a2c3a into main Feb 14, 2025
16 checks passed
@SteffenDE SteffenDE deleted the sd-upload-progress-redirect branch February 14, 2025 19:20
SteffenDE added a commit that referenced this pull request Feb 18, 2025
When someone does a push_navigate in the progress callback of an upload,
the render_upload function of LiveViewTest sometimes did not properly
return the live_redirect error tuple. This happened because the Upload-
Channel closes as soon as an upload is consumed. When this happens in the
progress callback, the test code and the channel code raced:
The channel sends a redirect message to itself (or the root view) which
shuts down once it receives it. The test code triggers a new render
through the ClientProxy. Now it could happen that:

1. the test process can successfully call the ClientProxy, but the
   ClientProxy then crashed while pinging the channel
2. or the ClientProxy succeeds in pinging and actually returns the new
   HTML while the test expects the redirect error tuple
3. or the channel and client proxy shut down in time for the test process
   to fail rendering and return the error tuple as expected

We fix this race by ensuring that we ping the view (and root view) as
soon as the upload is finished. This ensures that the channel has time to
send the redirect message before the render call can ping it again. Thus,
we always get the expected shutdown at the latest when the render call
pings the channel.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

push_navigate in handle_progress callback of LiveView Upload causes Floki exception in Test
2 participants