Fix "redefining module" warnings in test suite

[iancanderson]

I noticed that there are a bunch of warnings when running the test suite like this:

warning: redefining module Apartmentex.TestPostgresRepo.Migrations.CreateTenantUser (current version defined in memory)
  priv/repo/tenant_migrations/20160711125401_test_create_tenant_notes.exs:1

I think this has to do with the way the Ecto.Migrator is loading the code in the migration files, but I'm not sure if the fix should be within Ecto or within Apartmentex. Thoughts?

Here's where the migrator loads the code in the migration files:
https://github.com/elixir-ecto/ecto/blob/master/lib/ecto/migrator.ex#L239

[iancanderson]

FYI I'm getting similar warnings in the app that uses apartmentex, when I call Apartmentex.new_tenant within the test suite

[Dania02525]

Yeah I think this will be something we see since we are defining the same module twice by loading it with ecto migrator. I'm not sure if there are any performance implications of reloading a module for each tenant migration, but I'd guess they are small. for the tests, this ignores the warnings:
Code.compiler_options(ignore_module_conflict: true)

[Dania02525]

so changing the Code.load_file(...) in Ecto to Code.require_file(...) would prevent the file being reloaded if already loaded. I'm not sure there is a reason require_file couldn't be used in this situation, as I can't see why Ecto would need to reload a migration that has not yet run.

Ok, so the above doesn't work since when a file is already loaded, require_file returns nil which is not enumerable. Really what migrate/4 should be enumerating is the loaded migration modules, rather than the actual file migrations. Having said that, we'd need an agent to keep track of the loaded migration modules across migrations that may happen during app runtime, and it should start with ecto or the app. We aren't able to use the runner process from /lib/ecto/migration/runner.ex either, since it only calls start_link at the time it is invoked by the migrator loading the files, and is transient to each time migrate is called. I'm thinking to hang onto already loaded migration modules in a long running, supervised process so we don't load them again, but this is probably outside the scope of ecto itself. I can't see another way to do this without bringing back Apartmentex.Migration. What do you think?
edit: maybe we can use :code.all_loaded to get us the loaded modules, then parse through them for 'migration'? I'm caught by this bug: bitwalker/combine#17 on my dev computer so elixir is broken at the moment 👎

[brandonparsons]

@Dania02525 @iancanderson Did you guys figure this out a way to suppress these warnings? I'm getting the same.

[Dania02525]

@brandonparsons you should be able to ignore the warnings by putting this: Code.compiler_options(ignore_module_conflict: true) somewhere in your test_helpers.exs. Let me know if you run into it when compiling the dev application, as this might indicate something that would break distillery/exrm releases

[brandonparsons]

Thanks - yes this does stop the warnings in test. I'm not seeing any of these errors cropping up when poking around in iEx (that's as close as I have to a dev application at the moment).

[brandonparsons]

Actually I take that back - I do see these in the iEx console when adding new tenants and it tries to migrate their respective schemas. I did not set Code.compiler_options(ignore_module_conflict: true) for dev, as I wasn't so sure if this was a good idea or not.

[brandonparsons]

@Dania02525 I believe this is probably related, but under certain configurations, I think this is causing errors to be thrown.

** (CompileError) _build/test/lib/myapp/priv/repo/tenant_migrations/20161023051549_create_locations.exs:1: cannot define module Myapp.Repo.TenantMigrations.CreateLocations because it is currently being defined in _build/test/lib/myapp/priv/repo/tenant_migrations/20161023051549_create_locations.exs:1

If it helps - I'm creating two separate tenants inside the setup_all block of my tests. Something here is causing issues.

[brandonparsons]

Stacktrace:

     stacktrace:
       (elixir) src/elixir_module.erl:154: :elixir_module.build/5
       (elixir) src/elixir_module.erl:78: :elixir_module.do_compile/5
       (stdlib) erl_eval.erl:670: :erl_eval.do_apply/6
       (elixir) src/elixir.erl:223: :elixir.erl_eval/3
       (elixir) src/elixir.erl:211: :elixir.eval_forms/4
       (elixir) src/elixir_compiler.erl:66: :elixir_compiler.eval_compilation/3
       (elixir) src/elixir_lexical.erl:17: :elixir_lexical.run/3
       (elixir) src/elixir_compiler.erl:30: :elixir_compiler.quoted/3
       (elixir) lib/code.ex:321: Code.load_file/2
       (ecto) lib/ecto/migrator.ex:239: anonymous fn/4 in Ecto.Migrator.migrate/4
       (elixir) lib/enum.ex:1184: Enum."-map/2-lists^map/1-0-"/2
       (elixir) lib/enum.ex:1184: Enum."-map/2-lists^map/1-0-"/2
       lib/apartmentex/tenant_actions.ex:72: Apartmentex.TenantActions.handle_database_exceptions/1
       lib/apartmentex/tenant_actions.ex:57: Apartmentex.TenantActions.migrate_and_return_status/4
       (myapp) lib/myapp/operators/actions.ex:169: 
Myapp.Operators.Actions.create_schema/1
       (ecto) lib/ecto/multi.ex:386: Ecto.Multi.apply_operation/5
       (elixir) lib/enum.ex:1623: Enum."-reduce/3-lists^foldl/2-0-"/3
       (ecto) lib/ecto/multi.ex:376: anonymous fn/5 in Ecto.Multi.apply_operations/5
       (ecto) lib/ecto/adapters/sql.ex:508: anonymous fn/3 in Ecto.Adapters.SQL.do_transaction/3
       (elixir) src/elixir.erl:223: :elixir.erl_eval/3

[FabienHenon]

I also got this issue, and another one talking about deadlocks. It seems to be because of the migration beeing executed in parallel in the tests. I set async: false in my tests and the errors are gone.

But wouldn't it be possible that we get these errors again in dev or production if multiple tenants are created at the same time?

[FabienHenon]

I can confirm with a small code:

1..10
|> Task.async_stream(fn id -> Apartmentex.new_tenant(Repo, id) end)
|> Enum_to_list

That creating a tenant cannot be done asynchronously :(
If there is no other way to do, I'll have to forget Apartmentex and using my company_id filtering in each of my queries :(

[Dania02525]

Are you seeing a Postgres error, meaning a table is locked, or an elixir/erlang error? The migrators are separate processes, and the actual migrations should happen in separate schemas (obviously), so maybe we're getting a lock on writing to the tenants table in the public schema?

[FabienHenon]

I don't remember exactly and I totally changed my code :(
But as far as I remember the errors where from Postgres (I already got these kinds of errors with postgres in another project using Hibernate), but reported from Ecto

[BenMorganIO]

This is from Ecto when it loads the file:

https://github.com/elixir-ecto/ecto_sql/blob/master/lib/ecto/migrator.ex#L443

As we can see, each time the migrator runs, it executes Code.load_file/1 which, if ran more than once, will return an error. It's counterpart, Code.require_file/1 will return nil if a file is already loaded. If anyone knows a solution that will, if ran a second time, just return modules defined in the file, then we should get a PR over to Ecto to solve this.

Link to the doc: https://hexdocs.pm/elixir/1.6.6/Code.html#load_file/2

[BenMorganIO]

Actually, now that I think of it, Ecto could use Code.eval_file/2.