Mix tasks that expect configuration no longer work

[ankhers]

Since we moved to the cache which requires starting a genserver, things like mix ecto.create no longer work because the ets table has not been started.

We could just get people to create an alias for these kinds of tasks that prepends app.start, but then you also get a bunch of errors saying that the database does not exist when the app tries to start. This kind of thing might be okay for a first run on a developer machine, but on CI where you start with a fresh database every run it would get annoying having this in your logs.

[ankhers]

@sasa1977 I would love to hear your thoughts on this.

[sasa1977]

Arguably, this is a more general problem. Suppose you need to read secrets from some external storage. To do that, you need to start the client process. And now you end up with the same problem.

I guess folks don't experience this in practice, since they probably read external config only in prod.

The easiest solution would be to start_link the config process from the init callback if context is :runtime (which AFAIK happens only when running mix ecto tasks).

[ankhers]

That doesn't seem to work. It seems that mix ecto.migrate will attempt to start the repo with :supervisor and the config process has not yet started. So we just delay the problem. I was able to fix it using the following macro,

  defmacro __using__(opts) do
    config_module = opts[:config_module]
    options = opts[:options]

    quote bind_quoted: [config_module: config_module, options: Macro.escape(options)] do
      def init(_context, config) do
        pid = Process.whereis(unquote(config_module))

        if is_nil(pid) do
          unquote(config_module).start_link([])
        end

        config = {:ok, Keyword.merge(config, unquote(options))}

        if is_nil(pid) do
          GenServer.stop(unquote(config_module))
        end

        config
      end
    end
  end

Which will start and stop the config process only if it is not already started. This could become something like

defmodule MyApp.Repo do
  use Ecto.Repo, ...

  use Provider.EctoRepo, 
    config_module: MyApp.Config,
    options: [hostname: MyApp.Config.hostname(), ...]
end

Admittedly, I'm not super happy with this. I feel like there should be a simpler way to approach this. Probably worth reaching out to the ecto team to see if they have any general guidance for something like this.

[sasa1977]

Ah, make sense.

Generating init in the repo is a bit meh, because the user might have their own init defined. But, we could defin ensure_started function in the config module, which would do something like:

def ensure_started do
  case start_link(nil) do
    {:ok, pid} -> pid
    {:error, {:already_started, pid}} -> pid
  end
end

And then you just need to call this function from the Repo's init callback, regardless of the context.
Would that work?

[ankhers]

No, this will have the same issue as I was having before. This will end up starting the process, then when the application starts and the supervisor attempts to start the process, it will already exist.

If I do something similar in the start_link/1 function, convert the already started error into {:ok, pid}, everything works as you would expect. But I'm not really sure if there are drawbacks to doing that.

[sasa1977]

I don't understand how? The ensure_started version should succeed even if the process is already running, because it explicitly handles that error and converts it into a success.

[ankhers]

Right. It will always start from within the repo. However, the test mix task starts your application. So the order of events becomes

1. The ecto.create task runs and attempts to start the repo, so we start the config process along with it
2. The ecto.migrate task also attempts to start the repo, so we "start" the config process along with it (in quotes because it is already started in step 1)
3. The test task starts your app, which will start your supervisor, which will attempt to start the config process. However, it was already started in step 1 and the supervisor crashes because the process already exists.

[sasa1977]

Oh, I see. That's starting to make sense now 😅
Could you try the following in a hardcoded way:

1. in init/1 ensure that the config process is started (regardless of context)
2. then at the end of init/1 stop it only if context is runtime

If this works, then we could provide a wrapper fun, something like

def init(context, config) do
  Provider.ecto_config(MyConfig, context, fn ->
    # ecto_config will ensure that MyConfig is started, and it will stop it if context is runtime

    # whatever we return from this function is also returned by ecto_config
  end)
end

[ankhers]

That does seem to work.

If you feel like throwing together a PR for it, I'd be happy to accept it. If you can't find the time, I will just do it in the morning when I get up.

Thanks for helping me through this one.

[ankhers]

Resolved from #7