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

Application modules list in app controller not reloaded after compile #13001

Closed
lukaszsamson opened this issue Oct 9, 2023 · 8 comments
Closed

Comments

@lukaszsamson
Copy link
Contributor

Elixir and Erlang/OTP versions

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

Elixir 1.15.6 (compiled with Erlang/OTP 26)

Operating system

macOS

Current behavior

Repro:

  1. mix new --sup app_modules_bug
  2. cd app_modules_bug
  3. add an empty module to lib/app_modules_bug.ex
defmodule AppModulesBug.Foo do
end
  1. run this in iex
iex(1)> Mix.start()
:ok
iex(2)> Code.compile_file("mix.exs")
[
  {AppModulesBug.MixProject,
   <<70, 79, 82, 49, 0, 0, 7, 200, 66, 69, 65, 77, 65, 116, 85, 56, 0, 0, 0,
     247, 0, 0, 0, 24, 31, 69, 108, 105, 120, 105, 114, 46, 65, 112, 112, 77,
     111, 100, 117, 108, 101, 115, 66, 117, 103, 46, 77, ...>>}
]
iex(3)> Mix.Task.run("compile", [])
Compiling 2 files (.ex)
Generated app_modules_bug app
{:ok, []}
  1. verify app modules
iex(4)> :application.get_key(:app_modules_bug, :modules)
{:ok, [AppModulesBug, AppModulesBug.Application, AppModulesBug.Foo]}
  1. rename AppModulesBug.Foo to AppModulesBug.Bar & save
  2. run
iex(5)> Mix.Task.clear()
:ok
iex(6)> Mix.Task.run("compile", [])
Compiling 1 file (.ex)
Generated app_modules_bug app
{:ok, []}
  1. verify app modules
iex(7)> :application.get_key(:app_modules_bug, :modules)
{:ok, [AppModulesBug, AppModulesBug.Application, AppModulesBug.Foo]}

Notice it returns the old module AppModulesBug.Foo and does not return the new module AppModulesBug.Bar

Expected behavior

I expect that after a successful compile app manifests for all generated apps will be reloaded and app controller will return correct app metadata

@josevalim
Copy link
Member

This is expected. The compiler will not stop, unload, load, and restart all applications that are currently running. It only compiles the new code. If you need updated application entries, then you need to stop, unload, load, and start them back up. :)

@josevalim josevalim closed this as not planned Won't fix, can't repro, duplicate, stale Oct 9, 2023
@lukaszsamson
Copy link
Contributor Author

@josevalim Then it is inconsistent behaviour as it loads the apps on the first compile, even if it is a noop

iex(1)> Mix.start()
:ok
iex(2)> Code.compile_file("mix.exs")
[
  {AppModulesBug.MixProject,
   <<70, 79, 82, 49, 0, 0, 7, 200, 66, 69, 65, 77, 65, 116, 85, 56, 0, 0, 0,
     247, 0, 0, 0, 24, 31, 69, 108, 105, 120, 105, 114, 46, 65, 112, 112, 77,
     111, 100, 117, 108, 101, 115, 66, 117, 103, 46, 77, ...>>}
]
iex(3)> :application.get_key(:app_modules_bug, :modules)
:undefined
iex(4)> Mix.Task.run("compile", [])
{:noop, []}
iex(5)> :application.get_key(:app_modules_bug, :modules)
{:ok, [AppModulesBug, AppModulesBug.Application, AppModulesBug.Bar]}

Even if that is intended then it is not easy to guess which apps need to be reloaded. The compiler is not returning that info and parsing stdout for Generated app_modules_bug app is not an option

@josevalim
Copy link
Member

@lukaszsamson you know the current application name and you can use :application.get_key(:app_modules_bug, :applications) to find all applications and their dependencies recursively.

I think that's not the main point though. The issue is that, if you start the runtime, then shutting all apps down and loading it all up will be very expensive. Restarting the VM will most likely be faster.

Then it is inconsistent behaviour as it loads the apps on the first compile, even if it is a noop

Yes, the current projects and dependencies have to be loaded for validation and other stuff. But we are not checking in any way if those definitions are current, stale, etc. We only ensure they are loaded. You can pass "--no-app-loading" if you really want to avoid loading it, but then you don't get Application.compile_env validation.

@josevalim
Copy link
Member

josevalim commented Oct 9, 2023

Let me give more examples. Within the same VM instance, we won't check several things:

  1. if you change the mix.exs, we don't pick it up
  2. if you change a dependency source file on disk, we don't pick it up
  3. if you dynamically change a module in memory, we won't detect it as changed
  4. if you programmatically change the load paths and swap one application for another version behind the scenes

Mix.Task.clear() is not going to undo all of the side-effect done by tasks and checking or undoing those side-effects would be too expensive, potentially making every Elixir app take minutes to boot, for a use case that is not the primary way Elixir apps are commanded.

@lukaszsamson
Copy link
Contributor Author

Well, that would explain some of the weirdness (e.g. the infamous (Mix.Error) Can't continue due to errors on dependencies elixir-lsp/elixir-ls#597).
ElixirLS (and iex for that matter) relies on :application.get_key(app, :modules). I guess the safest solution then is unloading everything before running compile.

@josevalim
Copy link
Member

ElixirLS (and iex for that matter) relies on :application.get_key(app, :modules)

What do you use it for?

I guess the safest solution then is unloading everything before running compile.

Per above, unloading everything will be very expensive (and speaking from experience). And you will run into issues like, if there is a WebSocket connection, then you need to wait for the webserver timeout, etc. The best way would probably be to decouple the language server from the instance that compiles/builds the user code. Then, if necessary, you start a new instance at any time. Throwing everything out is typically faster than undoing and redoing.

lukaszsamson added a commit to elixir-lsp/elixir-ls that referenced this issue Oct 9, 2023
we rely on application controller being able to return app modules
elixir loads apps only on initial compilation and and without purging we end up with outdated module list
workaround elixir-lang/elixir#13001
@lukaszsamson
Copy link
Contributor Author

What do you use it for?

To get application by module, to get a list of modules for completions (here's an example from iex

:ets.match(:ac_tab, {{:loaded, :"$1"}, :_})
), to get all modules implementing a behaviour.

unloading everything will be very expensive

That's unfortunate but as a general rule ElixirLS is not starting client apps (custom compilers are an exception), only loads.

The best way would probably be to decouple the language server from the instance that compiles/builds

Even if we go down this road I'd prefer to see a mix compile --watch mode and that would IMO need to reload apps correctly

@josevalim
Copy link
Member

But in IEx we complement it with this, which will also show any loaded module, including ones which may be compiled in later:

modules = Enum.map(:code.all_loaded(), &Atom.to_string(elem(&1, 0)))

lukaszsamson added a commit to elixir-lsp/elixir-ls that referenced this issue Jan 8, 2024
fixes an issue when workspace symbols would not see a newly added module due to app controller keeping old module list
Workaround for elixir-lang/elixir#13001
lukaszsamson added a commit to elixir-lsp/elixir-ls that referenced this issue Jan 8, 2024
fixes an issue when workspace symbols would not see a newly added module due to app controller keeping old module list
Workaround for elixir-lang/elixir#13001
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants