elixir-geolix / geolix

IP information lookup provider
Apache License 2.0
190 stars 18 forks source link

0.16 crashes at startup #19

Closed Nicd closed 6 years ago

Nicd commented 6 years ago

I upgraded my codebase to Geolix 0.16.0 and it no longer starts up, instead it crashes on:

[info] Application geolix exited: Geolix.start(:normal, []) returned an error: exited in: GenServer.call(Geolix.Database.Loader, {:load_database, %{adapter: Geolix.Adapter.MMDB2, id: :city, source: "/Users/nicd/svn/code-stats/_build/dev/lib/code_stats/priv/geoip-cities.gz"}}, :infinity)
    ** (EXIT) no process: the process is not alive or there's no process currently associated with the given name, possibly because its application isn't started

I did not change anything related to the startup procedure from 0.15.1.

My config:

config :geolix, init: {CodeStatsWeb.Geolix, :init}

The init module:

defmodule CodeStatsWeb.Geolix do
  @moduledoc """
  Module for initialising Geolix databases at runtime instead of config time.
  """

  def init() do
    priv_dir = Application.app_dir(:code_stats, "priv")

    Geolix.load_database(%{
      id: :city,
      adapter: Geolix.Adapter.MMDB2,
      source: Path.join([priv_dir, "geoip-cities.gz"])
    })

    Geolix.load_database(%{
      id: :country,
      adapter: Geolix.Adapter.MMDB2,
      source: Path.join([priv_dir, "geoip-countries.gz"])
    })
  end
end

If I put prints to that init module, I can see that it is called. I tried adding the Geolix supervisor to my main supervisor's child list and adding :geolix to extra_applications, but it still happens. If I downgrade to 0.15.1, it works like it used to. Did I miss something?

For now I have changed my configuration to the new style of:

config :geolix,
  databases: [
    %{id: :city, init: {CodeStatsWeb.Geolix, :init_cities}},
    %{id: :country, init: {CodeStatsWeb.Geolix, :init_countries}}
  ]

where init_cities and init_countries return the configuration. But I'm left wondering why my earlier way doesn't work. I didn't see an outright deprecation in the changelog.

mneudert commented 6 years ago

I think the problem is the initialization flow. The global init method is called before any supervisor children are started, therefore also before the loader is available for calling.

Can you try to change your load_database/1 calls to Application.put_env/3 and only modify the environment instead of directly loading? That would be the intended usage (and might only need some further clarification in the documentation).

Nicd commented 6 years ago

I can try that when I am home. But why did it work earlier, in 0.15.1? Just by accident?

mneudert commented 6 years ago

Probably by chance with some sort of duplicate configuration. Or the results where just empty.

As the whole init logic is only available since 0.16.0 it should have been simply ignored. And then at least not broken something.

Nicd commented 6 years ago

Ok, so I did miss something then. Sorry for bothering you, but thanks for the help. :) Maybe it would be good to document what the my_init_fun function should look like since it's not mentioned right now in the README (when using config :geolix, init: { MyInitModule, :my_init_fun })?

ntodd commented 6 years ago

Had some issues with this as well. Don't think the documentation is super clear about how to dynamically load the paths at runtime (at least not to someone starting Elixir/Phoenix about 4 hours ago 😃 ).

I was able to get everything loading at runtime with the following:

In lib/myapp/geolix.ex

defmodule Iplix.Geolix do
  @moduledoc """
    Module for initializing geolix db path environment variables at runtime
  """
  def init() do
    System.put_env("geolix_db_cities", db_path("GeoLite2-City.mmdb.gz"))
    System.put_env("geolix_db_countries", db_path("GeoLite2-Country.mmdb.gz"))
  end

  defp db_path(db_name) do
    Path.join([Application.app_dir(:iplix, "priv"), "data", db_name])
  end
end

Then, in config.exs

config :geolix,
  init: {Iplix.Geolix, :init},
  databases: [
    %{
      id: :city,
      adapter: Geolix.Adapter.MMDB2,
      source: {:system, "geolix_db_cities"}
    },
    %{
      id: :country,
      adapter: Geolix.Adapter.MMDB2,
      source: {:system, "geolix_db_countries"}
    }
  ]

Things that caught me up:

  1. The mention of Application.put_env/3 above where it looks like you should be doing System.put_env/2 instead.
  2. Readme does not make it clear that you need to call the init to create the env vars and then use them in the database config instead of doing load_database/1 as @Nicd tried. It's not clear what the init should do.

This seems to work, but I have no idea if there is a better way.

mneudert commented 6 years ago

If have (finally!) rewritten most of the README documentation to hopefully be more resourceful regarding the various configurations, including some examples for the dynamic configuration. I would not call it finished but it should at least be more useful than before. For example the exact workflow (and timing) of "startup -> init -> load" may lack some more details.

@ntodd you should not need to use :system at all with the initializers (as freshly documented ^^).

Top level initialization:

defmodule Iplix.Geolix do
  def init() do
    Application.put_env(:geolix, :databases, [
      %{
        id: :city,
        adapter: Geolix.Adapter.MMDB2,
        source: db_path("GeoLite2-City.mmdb.gz")
      },
      %{
        id: :country,
        adapter: Geolix.Adapter.MMDB2,
        source: db_path("GeoLite2-Country.mmdb.gz")
      }
    ])
  end

  defp db_path(db_name) do
    Path.join([Application.app_dir(:iplix, "priv"), "data", db_name])
  end
end

config :geolix,
  init: {Iplix.Geolix, :init}

Per database initialization:

defmodule Iplix.Geolix do
  def init_database(%{id: :city} = database) do
    %{database | source: db_path("GeoLite2-City.mmdb.gz")}
  end

  def init_database(${id: :country} = database) do
    %{database | source: db_path("GeoLite2-Country.mmdb.gz")}
  end

  defp db_path(db_name) do
    Path.join([Application.app_dir(:iplix, "priv"), "data", db_name])
  end
end

config :geolix,
  databases: [
    %{
      id: :city,
      adapter: Geolix.Adapter.MMDB2,
      init: {Iplix.Geolix, :init_database}
    },
    %{
      id: :country,
      adapter: Geolix.Adapter.MMDB2,
      init: {Iplix.Geolix, :init_database}
    }
  ]

That SHOULD work if nothing is horribly broken...

mneudert commented 6 years ago

I consider this solved after the new version has been released with the updated documentation. If any problems arise please comment here or open another issue.