[WIP] Use zeitwerk on load instead of hooking descendants subclasses #22945
Conversation
|
@miq-bot cross-repo-tests /all ManageIQ/manageiq-ui-classic#9119 |
From Pull Request: ManageIQ/manageiq#22945
Fryguy
changed the title
| child.safe_constantize | ||
| rescue NameError => err |
There was a problem hiding this comment.
Can safe_constantize actually raise a NameError? I thought under the covers they caught that and just returned nil.
There was a problem hiding this comment.
yes, it actually did, thanks for reminding me. I need to investigate why.
There was a problem hiding this comment.
So, the short of it is that some top level classes can introduce cycles if you try to constantize their children. Their children can reference constants that get loaded and loading of mixins and perent classes and some of those kick off more constantizing of children classes and some of those can be cyclic loads. So far,MiqDecorator, OrchestrationStack::Status, and ResourcePool look to be the problems.
There was a problem hiding this comment.
Ewwww ResourcePool is bad. We need to fix that
There was a problem hiding this comment.
yeah, I'm getting things green with the current code and will try to figure out how to fix these classes or fix how we load.
We're now eager loading subclasses when loading the parent but this is far less reliant on hacks and closer to full eager loading, which is what rails has been pushing us to do for years. Let's try it out. Rails changed in 7.0 to call subclasses from reload_schema_from_cache here: rails/rails@6f30cc0 It also changed to call descendants on the callback class (self) insead of the ActiveSupport::DescendantsTracker here: rails/rails@ffae3bd We are not expecting to be called from these locations. Avoiding descendants / subclasses calls in conditionals like this are just brittle so we need to stop hooking through these methods: if !%w[reload_schema_from_cache __update_callbacks descendants subclasses].include?(caller_locations.first.base_label)
jrafanie
force-pushed
the
use_zeitwerk_on_load_instead_of_hooking_descendants_subclasses
branch
from
e54f435
to
f0715ae
Compare
|
Some comments on commits jrafanie/manageiq@cdb773d~...f0715ae config/initializers/zeitwerk.rb
|
|
Checked commits jrafanie/manageiq@cdb773d~...f0715ae with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint config/initializers/zeitwerk.rb
lib/extensions/descendant_loader.rb
|
| DescendantLoader.instance.discovered_parent_child_classes.each do |parent, children| | ||
| next if excluded_from_eager_load.include?(parent) | ||
|
|
||
| puts "Registering on_load for class: #{parent}" |
There was a problem hiding this comment.
I know this is for debug but I wonder if we should keep it and use $log.debug instead. (Same comment for the other puts lines)
WIP - don't merge yet