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

runtime checks for socket access in async functions #3161

Conversation

SteffenDE
Copy link
Collaborator

Followup for #3160.

Co-authored-by: José Valim <jose.valim@gmail.com>
Comment on lines 13 to 23
defmodule AssignAsyncSocket do
use Phoenix.LiveView

def mount(_params, _session, socket) do
{:ok, #{unquote(fun)}(socket, :foo, fn ->
do_something(socket.assigns)
end)}
end

defp do_something(_socket), do: :ok
end
Copy link
Member

@josevalim josevalim Mar 7, 2024

Choose a reason for hiding this comment

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

You can probably use eval_quoted. The if false is because we want the code expanded but not executed. :)

Suggested change
defmodule AssignAsyncSocket do
use Phoenix.LiveView
def mount(_params, _session, socket) do
{:ok, #{unquote(fun)}(socket, :foo, fn ->
do_something(socket.assigns)
end)}
end
defp do_something(_socket), do: :ok
end
require Phoenix.LiveView
if false do
Phoenix.LiveView.#{unquote(fun)}(socket, :foo, fn ->
do_something(socket.assigns)
end)
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.

No luck with eval_quoted.

ExUnit.start()

defmodule MyTest do
  use ExUnit.Case

  import ExUnit.CaptureIO

  for fun <- [:assign_async, :start_async] do
    test "warns when passing socket to #{fun} function" do
      warnings =
        capture_io(:stderr, fn ->
          Code.eval_quoted(
            quote bind_quoted: [fun: unquote(fun)] do
              require Phoenix.LiveView

              if false do
                fun(socket, :foo, fn ->
                  do_something(socket.assigns)
                end)
              end
            end
          )
        end)

      assert warnings =~
               "you are accessing the LiveView Socket inside a function given to #{unquote(fun)}"
    end
  end
end
mix run test.exs


  1) test warns when passing socket to start_async function (MyTest)
     test.exs:9
     ** (CompileError) nofile: cannot compile file (errors have been logged)
     stacktrace:
       (elixir 1.16.1) src/elixir_clauses.erl:45: :elixir_clauses.clause/6
       (elixir 1.16.1) src/elixir_clauses.erl:347: anonymous fn/7 in :elixir_clauses.expand_clauses_origin/6
       (stdlib 5.2) lists.erl:1706: :lists.mapfoldl_1/3
       (stdlib 5.2) lists.erl:1707: :lists.mapfoldl_1/3
       (elixir 1.16.1) src/elixir_clauses.erl:351: :elixir_clauses.expand_clauses_origin/6
       (stdlib 5.2) lists.erl:1706: :lists.mapfoldl_1/3
       (elixir 1.16.1) src/elixir_clauses.erl:98: :elixir_clauses.case/4
       (elixir 1.16.1) src/elixir_expand.erl:692: :elixir_expand.expand_case/5
       (elixir 1.16.1) expanding macro: Kernel.if/2
       nofile:1: (file)



  2) test warns when passing socket to assign_async function (MyTest)
     test.exs:9
     ** (CompileError) nofile: cannot compile file (errors have been logged)
     stacktrace:
       (elixir 1.16.1) src/elixir_clauses.erl:45: :elixir_clauses.clause/6
       (elixir 1.16.1) src/elixir_clauses.erl:347: anonymous fn/7 in :elixir_clauses.expand_clauses_origin/6
       (stdlib 5.2) lists.erl:1706: :lists.mapfoldl_1/3
       (stdlib 5.2) lists.erl:1707: :lists.mapfoldl_1/3
       (elixir 1.16.1) src/elixir_clauses.erl:351: :elixir_clauses.expand_clauses_origin/6
       (stdlib 5.2) lists.erl:1706: :lists.mapfoldl_1/3
       (elixir 1.16.1) src/elixir_clauses.erl:98: :elixir_clauses.case/4
       (elixir 1.16.1) src/elixir_expand.erl:692: :elixir_expand.expand_case/5
       (elixir 1.16.1) expanding macro: Kernel.if/2
       nofile:1: (file)


Finished in 0.01 seconds (0.01s on load, 0.00s async, 0.00s sync)
2 tests, 2 failures

Randomized with seed 369048

“errors have been logged”

Copy link
Member

Choose a reason for hiding this comment

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

What about:

          Code.eval_quoted(
            quote do
              require Phoenix.LiveView

              if false do
                unquote(fun)(socket, :foo, fn ->
                  do_something(socket.assigns)
                end)
              end
            end

Copy link
Member

Choose a reason for hiding this comment

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

Actually, sorry:

        Code.eval_quoted(
            quote do
              require Phoenix.LiveView

              if false do
                Phoenix.LiveView.unquote(fun)(socket, :foo, fn ->
                  do_something(socket.assigns)
                end)
              end
            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.

Nope. No luck with that either. But I know why I get no compiler errors... It's inside the capture_io.

Removing it yields

error: undefined variable "socket" (context MyTest)
└─ nofile:1

which kind of makes sense.

Besides that I think the defmodule code is actually alright. Might even be better to understand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added some eval_quoted variants as well. Let me know which ones you like better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@SteffenDE SteffenDE force-pushed the warn_on_socket_in_handle_async_runtime branch from ed913ac to 8486f10 Compare March 7, 2024 20:33
SteffenDE added a commit to phoenixframework/phoenix that referenced this pull request Mar 8, 2024
SteffenDE added a commit to phoenixframework/phoenix that referenced this pull request Mar 8, 2024
@chrismccord chrismccord merged commit 7f6a9c8 into phoenixframework:main Mar 11, 2024
5 checks passed
@chrismccord
Copy link
Member

❤️❤️❤️🐥🔥

chrismccord pushed a commit to phoenixframework/phoenix that referenced this pull request Mar 11, 2024
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.

None yet

3 participants