Consider reducing ensure_compiled checks

[josevalim]

Hi @danschultzer!

Apologies this the wrong repo, but I am relaying a bug report done on Ecto. The mechanism Pow currently uses to discover modules may considerably slow down compilation, as each ensure_compiled call to a module that does not exist will halt the compilation of the current module until all other modules in the system are compiled. Since you are performing this check to find guesses, it will halt in many cases. If one module happens to check the other that is also running the same check, they will deadlock. This is a circular dependency which will likely emit warnings in the future. I would drop the implicit checks and guessing in favor of explicitly passing module names.

Thanks!

PS: I will improve the docs for ensure_compiled with these notes as well.

[liamwhite]

relevant: #173

[josevalim]

I also would like to add that the guess may emit false negatives: i.e. it will say that a module is not available when it was just not made available yet.

[danschultzer]

Thanks for letting me know!

As @liamwhite points out there have been issues with the Code.ensure_compiled?/1 call before with #173.

I agree that explicitly passing module names would be the best. After working on it for a bit I don't see any easy way of doing that though (e.g. how Pow.Extension.Ecto.Schema.attrs/1 and Pow.Extension.Phoenix.ControllerCallbacks are used makes it difficult).

The reason why I'm using Code.ensure_compiled?/1 is that I need to know whether an extension has a certain module available, so I can filter out extensions that doesn't have said module. It would be easy enough to solve if I could just make the assumption that all extensions will have said module, but that won't work since you can pass otp_app: :my_app (pulling config from app env) instead of extensions: [...].

You may want to just define all extensions you want to use in one place.

Right now I've a few ideas:

1. For macros, check whether the passed opts has :extensions, and then just assume that the module exists, otherwise fallback to the current method.

   This might work well since pretty much all documentation shows :extensions rather than :otp_app option being passed. An exception will be raised at compile-time if an extension doesn't have said module.

   However for run-time it'll still be necessary to check whether the module exists or not. As an example, this happens in Pow.Extension.Phoenix.ControllerCallbacks.

2. Make a base module for each extension that contains the relevant info on what modules the extension has. This would probably solve all issues above, but the disadvantage is that it's adding yet another module and that it might be easy to forget to update when changing features in extensions.

3. Use a try/catch to test if the module exists. I have no idea if it's a good idea though, and it probably won't solve the actual issue.

   try do
     module.__info__(:functions)
     true
   rescue
     _e in UndefinedFunctionError -> false
   end

I'll try a few things out and see what can work, and if there might be some other way to prevent having to call Code.ensure_compiled?/1.

[josevalim]

However for run-time it'll still be necessary to check whether the module exists or not. As an example, this happens in Pow.Extension.Phoenix.ControllerCallbacks.

For runtime, you don't need Code.ensure_compiled?, only Code.ensure_loaded?, which is already an improvement. However, Code.ensure_loaded? will also make things unreasonably slower if it is looking up for modules that do not exist. In a nutshell, these functions were written to ensure something is loaded, not to check if something is loaded or not. Usually, if something is not available, you should raise or log, so users don't depend on said slow path forever.

Make a base module for each extension that contains the relevant info on what modules the extension has.

This could be a good approach. To close the gap, you could have a test people would put in their apps and in the test you would traverse the base module and all modules in the system to see if anything was not configured properly.

test "uses all extensions" do
  Pow.validate_extensions_for(BaseModule)
end

This way you remove the validation concern from compile/runtime and into test.

Use a try/catch to test if the module exists. I have no idea if it's a good idea though, and it probably won't solve the actual issue.

Yup, try/catch would have exactly the same issues.

[danschultzer]

Resolved in #335 which adds a base module to all extensions that can be used to check what the extension supports. This should prevent all Code.ensure_compiled?/1 calls.

I did think about requiring explicit :extensions option for all the macros so they can fail at compile if the extension module doesn't exist, and I also think the test helper to validate that extensions configuration is a good idea.

However, that requires some more consideration for how config in Pow should work and how extensions should be structured, so I've opted to look into that at a later point.

Thanks for the help!