phoenixframework / phoenix

Peace of mind from prototype to production
https://www.phoenixframework.org
MIT License
21.27k stars 2.87k forks source link

Protocols no longer consolidated after CodeReloader executes #5831

Closed sb8244 closed 1 month ago

sb8244 commented 4 months ago

Environment

Erlang/OTP 26 [erts-14.1.1] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [jit]

Elixir 1.15.7 (compiled with Erlang/OTP 26)

Actual behavior

I utilize SomeProtocol.__protocol__(:impls) to retrieve the implementations of a protocol in my code base. This is to provide an automatic mapping of protocols for certain features. It works great, except when the CodeReloader runs.

When the CodeReloader runs, the protocols in my app become unconsolidated and I have to restart the dev server.

To trigger the CodeReloader, I make an update to an Elixir file and then refresh my app frontend.

I know there's a mix.exs setting regarding consolidation, but it is not present in my mix.exs file.

Expected behavior

Reloading the code should not result in protocols not being consolidated.

Workaround

I have been struggling with a workaround for this but finally decided to figure something out for it. I ended up with the following (very hacky) solution that does seem to work.

Effectively, I'm running the same code as the CodeReloader to consolidate protocols, but I have it in force mode so it doesn't noop.

defmodule Super.Util.Protocol do
  require Logger

  def impls_for(protocol_mod, opts \\ []) do
    case protocol_mod.__protocol__(:impls) do
      {:consolidated, impls} ->
        impls

      :not_consolidated ->
        if Keyword.get(opts, :raise) do
          raise "Protocols not consolidated. This can happen in dev (restart server). In production, this is not expected."
        else
          recompile_protocols(protocol_mod)
          impls_for(protocol_mod, Keyword.put(opts, :raise, true))
        end
    end
  end

  if Mix.env() == :dev do
    defp recompile_protocols(mod) do
      Logger.info("Recompiling protocols due to missing consolidation: #{mod}")
      Mix.Task.reenable("compile.protocols")
      Mix.Task.run("compile.protocols", ["--force"])
    end
  else
    defp recompile_protocols(_mod) do
      :ok
    end
  end
end
josevalim commented 3 months ago

Hi @sb8244! I cannot reproduce this issue. Here is what I did:

  1. Cloned latest Phoenix
  2. cd installer
  3. mix new demo --dev
  4. Added the following to lib/demo.ex:
defprotocol Foo do
  def omg(foo)
end

Now I started the server and whenever I change a file, I reload the page, triggering the code reloader. Both Foo and Enumerable are still consolidated.

Besides using latest Phoenix (main), I am also using Elixir v1.17.0.

sb8244 commented 3 months ago

@josevalim I will see if I can reproduce it on a clean repo. I don't think we're doing anything crazy (other than inspecting the protocol), but it's also a decently complex repo so there's a chance something is throwing it off.

jpbecotte commented 1 month ago

Hello @josevalim @sb8244 . We have the same problem. @josevalim You actually have to implement and call Foo.__protocol__(:impls) somewhere. And the bug will happen if you modify the implementation file (without of course breaking the implementation).

Here's my steps to reproduce:

  1. Cloned latest Phoenix
  2. cd installer
  3. mix new hello --dev
  4. Add a file my_protocol.ex:
defprotocol MyProtocol do
  def omg(foo)
end
  1. Add a file protocol_impl.ex:
defmodule SomeModuleImpl do
  defimpl MyProtocol, for: BitString do
    def omg(string) do
      string <> " (omg!)"
    end
  end
end
  1. Ad a file some_module.ex:
defmodule SomeModule do
  def some_fun do
    x = MyProtocol.__protocol__(:impls)
    inspect(x)
  end
end
  1. In lib/hello_web/controllers/page_html/home.html.heex, I have modified a little to call both the implementation and the introspecting function:
    <p class="text-[2rem] mt-4 font-semibold leading-10 tracking-tighter text-zinc-900 text-balance">
       <%= MyProtocol.omg("Peace of mind from prototype to production.") %>
       Implementations: <%= SomeModule.some_fun %>
    </p>
  1. Run mix phx.server. All is OK.

    Capture d’écran, le 2024-08-09 à 12 24 34
  2. Modify protocol_impl.ex, change (omg!) to (omgg!), for example.

  3. Reload the page.

Capture d’écran, le 2024-08-09 à 12 24 53
josevalim commented 1 month ago

Thank you, I was able to reproduce this issue and it is an Elixir bug. It is not hard to fix it, I should have something up tomorrow.

jpbecotte commented 1 month ago

Thank you!

sb8244 commented 4 weeks ago

Hey @josevalim , I patched this fix into my deps (so running 1.7.10 with this file patched) and re-compiled. I'm still seeing the issue pop up. It's hard to tell if it happens less or not—I will keep an eye on it. It does seem like when it happens, it is fixed on the next refresh.

My fear with reproduction is that we have quite a lot of protocol usage and a decently large app, so I'm not sure how easy it will be to reproduce.

Could part of the issue be the fix that I attempted for the problem?

defmodule Super.Util.Protocol do
  require Logger

  def impls_for(protocol_mod, opts \\ []) do
    case protocol_mod.__protocol__(:impls) do
      {:consolidated, impls} ->
        impls

      :not_consolidated ->
        if Keyword.get(opts, :raise) do
          raise "Protocols not consolidated. This can happen in dev (restart server). In production, this is not expected."
        else
          recompile_protocols(protocol_mod)
          impls_for(protocol_mod, Keyword.put(opts, :raise, true))
        end
    end
  end

  if Mix.env() == :dev do
    defp recompile_protocols(mod) do
      Logger.info("Recompiling protocols due to missing consolidation: #{mod}")
      Mix.Task.reenable("compile.protocols")
      Mix.Task.run("compile.protocols", ["--force"])
    end
  else
    defp recompile_protocols(_mod) do
      :ok
    end
  end
end
josevalim commented 4 weeks ago

If you have steps to reproduce the issue, I can take a look. What I can say is that the steps given above no longer show the issue.