rails-on-services / apartment

Database multi-tenancy for Rack (and Rails) applications
376 stars 146 forks source link

current tenant is lost when creating a new fiber #181

Open derikson opened 2 years ago

derikson commented 2 years ago

Steps to reproduce

  Apartment::Tenant.switch! 'db1'
  puts Apartment::Tenant.current                           # prints "db1"
  Fiber.new { puts Apartment::Tenant.current }.resume      # prints "public"
  puts (1..2).lazy.map { Apartment::Tenant.current }.peek  # prints "public"
  User.limit(2)
      .find_each(batch_size: 1)
      .lazy
      .map { puts Apartment::Tenant.current }.next         # prints "public"
  puts Apartment::Tenant.current                           # prints "db1"

Expected behavior

All of the above should print "db1".

Actual behavior

Inside a new fiber, the fiber local variable :apartment_adapter no longer exists, so it creates a new one, set to the default tenant. Since enumerators are implemented using fibers, some enumerator methods use the default tenant instead of the tenant of the parent fiber.

Apartment::Tenant.adapter was probably meant to use thread local variables, but Ruby 1.9.2 changed the meaning of Thread#[] and Thread#[]= to operate on fiber local variables instead of thread local variables. Instead Thread#thread_variable_get and Thread#thread_variable_set should be used.

System configuration

IlyaUmanets commented 2 years ago

Hey @derikson, I'm not a contributor but I will tell you my opinion.

I'd say it's expected behavior, I faced the same case on my project and we weren't surprised to see a public tenant under a new thread even though it was not public in a parent.

rpbaltazar commented 2 years ago

Hi @derikson, I don't have a use case for fibers so for me it's harder to make an informed decision. I do have a use case where I switch the DB connection from a writer to a reader db and I have the apartment setting the same tenant on the new connection. All of this to say, I do understand your concern and maybe we can make it configurable.

lunks commented 2 years ago

I think what @derikson presented here should be considered a bug according to his code. lazy shouldn't cause us to lose the current tenant.

derikson commented 2 years ago

@rpbaltazar I'm no longer working on a project that uses this gem, so I no longer have a use case that this solves. If nothing else, this bug report might help someone who is getting data from different tenants mixed together track down the cause.