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.

Sign up for GitHub

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

Jump to bottom

push_navigate in handle_progress callback of LiveView Upload causes Floki exception in Test #3662

Closed
PJUllrich opened this issue Feb 5, 2025 · 2 comments · Fixed by #3676
Closed

push_navigate in handle_progress callback of LiveView Upload causes Floki exception in Test #3662

PJUllrich opened this issue Feb 5, 2025 · 2 comments · Fixed by #3676

Comments

@PJUllrich
Copy link

Environment

  • Elixir version (elixir -v): 1.18.2
  • Phoenix version (mix deps): 1.7.19
  • Phoenix LiveView version (mix deps): 1.0.4
  • Operating system: macOS Sonoma 14.6.1
  • Browsers you attempted to reproduce this bug on (the more the merrier):
  • Does the problem persist after removing "assets/node_modules" and trying again? Yes/no:

Actual behavior

@SteffenDE This is a re-opening of this issue: #2047 (comment)

Here's a demo repo that replicates the bug: https://github.com/PJUllrich/phoenix-live-view-file-upload-test-bug
See this test: https://github.com/PJUllrich/phoenix-live-view-file-upload-test-bug/blob/main/test/demo_web/live/upload_live/show_test.exs

The bug does not always occur, because there's a race-condition between the LiveView shutting down (if you have a good way of catching this in the test and assert against it, i'd appreciate it by the way) and the re-rendering of the HTML. That's why I run the test 5 times, because then usually the bug occurs at least once.

In essence, if I mark a socket with push_navigate/2 inside the handle_progress callback of a LiveView upload, I receive this Floki exception:

  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)

Expected behavior

There's no Floki exception (and ideally I could assert against the live_redirect return of the LiveView in my test somehow like I can do with render_submit/2.
@PJUllrich
Copy link
Author

The same happens if you do a send(self(), :redirect) inside the handle_progress/3 callback and push redirect the socket in the handle_info(:redirect, socket) callback instead.

SteffenDE added a commit that referenced this issue Feb 13, 2025
@SteffenDE
fix race conditions when upload progress redirects
75d0588 
Because the channel did not immediately stop when an upload progress
handler redirected, the following could happen:

1. upload does a push_navigate
2. channel sends a :redirect message to the root_pid
3. render_upload tries to render the page

Now it depended on your luck:

4. LiveViewTest render asks the ClientProxy for the new HTML
5. ClientProxy tries to ping the channel
6. ClientProxy succeeds in pinging and returns HTML
7. Tests fail, because they expected {:error, {:live_redirect, ...}}

or

4. LiveViewTest render asks the ClientProxy for the new HTML
5. ClientProxy tries to ping the channel
6. ClientProxy exits, because ping crashes (channel already exited)

or

4. LiveViewTest render akss the ClientProxy for the new HTML
5. ClientProxy already exited

Fixes: #3662

I changed the code to immediately exit the channel when possible, therefore
at least the ping should always fail. I don't fully understand the
interactions at play here though and I don't really like the code.
@SteffenDE SteffenDE mentioned this issue Feb 13, 2025
Merged
SteffenDE added a commit that referenced this issue Feb 13, 2025
@SteffenDE
fix race conditions when upload progress redirects
fcd3e69 
Because the channel did not immediately stop when an upload progress
handler redirected, the following could happen:

1. upload does a push_navigate
2. channel sends a :redirect message to the root_pid
3. render_upload tries to render the page

Now it depended on your luck:

4. LiveViewTest render asks the ClientProxy for the new HTML
5. ClientProxy tries to ping the channel
6. ClientProxy succeeds in pinging and returns HTML
7. Tests fail, because they expected {:error, {:live_redirect, ...}}

or

4. LiveViewTest render asks the ClientProxy for the new HTML
5. ClientProxy tries to ping the channel
6. ClientProxy exits, because ping crashes (channel already exited)

or

4. LiveViewTest render akss the ClientProxy for the new HTML
5. ClientProxy already exited

Fixes: #3662

I changed the code to immediately exit the channel when possible, therefore
at least the ping should always fail. I don't fully understand the
interactions at play here though and I don't really like the code.
@PJUllrich
Copy link
Author

❤️💚💛💜💙❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix race conditions when upload progress redirects phoenixframework/phoenix_live_view
1 participant
@PJUllrich