Delayed Initialization is not thread-safe

[fsateler]

Steps to reproduce

I haven't been able to reproduce at will. Has happened in production though. Under some circumstances (high load during boot), excluded models are not correctly processed, and queries are made to the wrong schema. I believe #39 introduced the problem.

My current theory is that multiple threads are trying to init apartment at the same time, one reads @already_initialized as true, but process_excluded_models has not been called yet.

Expected behavior

Excluded models get queried against the public shcema:

SELECT "tentants".* from "public"."tenants"

Actual behavior

Excluded models get queried without schema.

SELECT "tentants".* from "tenants"

System configuration

This problems occurs on production, where puma is configured with multiple threads (5 max). This problem appears only intermittently, but always under high load.

• Database: Postgres 11

• Apartment version: 2.7.1

• Apartment config (in config/initializers/apartment.rb or so):

  • use_schemas: true

• Rails (or ActiveRecord) version: 6.0

• Ruby version: 2.6

Possible solutions

1. Revert Avoid early connection #39 and friends. This would reintroduce the problem that it is impossible to use any ruby code from the app without a working database connection (ie, during CI).
2. Somehow protect the initialization with a lock.
3. Add yet another hook that inits apartment before threads are started.
4. Deprecate excluded_models, in favor of a new method, acts_as_apartment or something like that, that does the model exclusion (I have not tried if this works).
5. Some other option?
6. Variation of 1, instead of revert, reintroduce the old behavior, behind an option (activated by default). This way, advanced users can disable it (and we can document this pitfall).

Locally, we have worked around the issue with 3:

on_worker_boot do
  ActiveRecord::Base.connection_pool.with_connection do
    Apartment::Tenant.init_once
  end
end

Thoughts?

[rybon]

Does this only happen with excluded models that make use of a shared schema? Are there any more thread-safety issues in this Gem? It's really worrying to see data from a different schema appear when it's not expected. That can have very serious consequences in certain use cases. The whole point of this Gem is to keep tenants strictly separated from each other unless explicitly defined otherwise and if that cannot be guaranteed due to for example threading issues it's a show stopper.

I appreciate all the hard work that went into this fork though! An issue like this is just really unsettling.

[rpbaltazar]

For now #118 i think solves the issue. we can try to work on a solution for the problems that the delayed initializer was trying to address.
Speaking for myself this has been a good learning about the gem itself as i was not involved in the development of this and was trying to move it forward since a lot of projects still depend on it.