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

Do not dynamically load tenant model specified in autoload. #228

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alpaca-tc
Copy link
Contributor

@alpaca-tc alpaca-tc commented Jan 18, 2024

The basic policy of Rails is that application code in the development environment is lazy loaded.
For this reason, class_name is passed as a string rather than a class in the reflection definition, and the string is not constantized until it is evaluated.
This is done to reduce performance issues and the complex locking load of constant-loading logic.

Now activerecord-multitenant is loading the class specified in the argument to multi_tenant() without lazy loading.
This conditional statement was added at #6, but I have been fixed.
Note that for projects where the tenant model does not exist, the existing behavior is supported by specifying skip_reflection: true.

The basic way of Rails is that application code in the development environment is lazy loaded.
For this reason, `class_name` is passed as a string rather than a class in the reflection definition, and the string is not constantized until it is evaluated.
This is done to reduce performance issues and the complex locking load of constant-loading logic.

Now activerecord-multitenant is loading the class specified in the argument to multi_tenant() without lazy loading.
This conditional statement was added at citusdata#6, but I have been fixed.
Note that for projects where the tenant model does not exist, the existing behavior is supported by specifying `skip_reflection: true`.
else
tenant_name.to_s.classify
end
!!class_name.safe_constantize
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class was dynamically loaded in this line.
Although top-level model can be checked with const_defined?(:X) or autoload?(:X), it is difficult for nested models such as A::B::Tenant, so I gave up the method of checking by the presence or absence of a class.

@alpaca-tc
Copy link
Contributor Author

Ruby can potentially cause deadlocks when loading code with multi-threading.
And in our project, this deadlock was occurring. rails/rails#50802

Part of the reason for this was that the gem was making constant load to Tenant in any multi-tenantized model.
Hopefully this PR will be merged.

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

Successfully merging this pull request may close these issues.

1 participant