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

Fix "redefining module" warnings in test suite #12

Open
iancanderson opened this issue Jul 20, 2016 · 15 comments
Open

Fix "redefining module" warnings in test suite #12

iancanderson opened this issue Jul 20, 2016 · 15 comments

Comments

@iancanderson
Copy link
Contributor

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
Copy link
Contributor Author

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

@Dania02525
Copy link
Owner

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
Copy link
Owner

Dania02525 commented Jul 27, 2016

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
Copy link

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

@Dania02525
Copy link
Owner

@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
Copy link

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
Copy link

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
Copy link

brandonparsons commented Nov 18, 2016

@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
Copy link

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
Copy link

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
Copy link

FabienHenon commented Aug 24, 2017

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
Copy link
Owner

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
Copy link

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
Copy link
Contributor

BenMorganIO commented Oct 6, 2018

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants