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

[WIP] Use zeitwerk on load instead of hooking descendants subclasses #22945

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/models/authenticator/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ module Authenticator
class Base
include Vmdb::Logging

# Simulate being a STI table class for proper descendant class loading
def self.inheritance_column
"type"
end

def self.validate_config(_config)
[]
end
Expand Down
3 changes: 0 additions & 3 deletions app/models/miq_request_workflow.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@ class MiqRequestWorkflow
include Vmdb::Logging
include DialogFieldValidation

# We rely on MiqRequestWorkflow's descendants to be comprehensive
singleton_class.send(:prepend, DescendantLoader::ArDescendantsWithLoader)

attr_accessor :requester, :values, :last_vm_id
attr_writer :dialogs

Expand Down
24 changes: 24 additions & 0 deletions config/initializers/zeitwerk.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,27 @@
Rails.autoloaders.main.collapse(Rails.root.join("lib/manageiq/reporting/charting"))
Rails.autoloaders.main.collapse(Rails.root.join("lib/ansible/runner/credential"))
Rails.autoloaders.main.collapse(Rails.root.join("lib/pdf_generator"))

excluded_from_eager_load = %w[
ApplicationRecord
MiqDecorator
OrchestrationStack::Status
ResourcePool
]

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}"
Copy link
Member

Choose a reason for hiding this comment

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

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)

Rails.autoloaders.main.on_load(parent.to_s) do |klass, _abspath|
puts "Running on_load for class: #{klass} with children: #{children}"
children.each do |child|
begin
child.safe_constantize
rescue NameError => err
Comment on lines +28 to +29
Copy link
Member

Choose a reason for hiding this comment

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

Can safe_constantize actually raise a NameError? I thought under the covers they caught that and just returned nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it actually did, thanks for reminding me. I need to investigate why.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Ewwww ResourcePool is bad. We need to fix that

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

puts "!!! FAILED to load #{child} for parent: #{klass} with inheritance column: #{klass.try(:inheritance_column)} reason: #{err}. Error class: #{err.class.name}"
raise
end
end
end
end
Empty file removed data/git_repos/locks/.gitkeep
Empty file.
5 changes: 5 additions & 0 deletions lib/acts_as_ar_model.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ def self.includes_to_references(_inc)
[]
end

# Simulate being a STI table class for proper descendant class loading
def self.inheritance_column
"type"
end

class << self; alias_method :base_model, :base_class; end

#
Expand Down
33 changes: 4 additions & 29 deletions lib/extensions/descendant_loader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,10 @@ def clear_class_inheritance_relationships
include Cache
include Mapper

def discovered_parent_child_classes
@discovered_parent_child_classes ||= class_inheritance_relationships.select { |_k, names| names.length > 0 }
end

def load_subclasses(parent)
names_to_load = class_inheritance_relationships[parent.to_s].dup
while (name = names_to_load.shift)
Expand All @@ -261,29 +265,6 @@ def scoped_name(name, scopes)
end
end

module ArDescendantsWithLoader
def descendants
if Vmdb::Application.instance.initialized? && !defined? @loaded_descendants
@loaded_descendants = true
DescendantLoader.instance.load_subclasses(self)
end

super
end

# Rails 6.1 added an alias for subclasss to call direct_descendants in:
# https://github.com/rails/rails/commit/8f8aa857e084b76b1120edaa9bb9ce03ba1e6a19
# We need to get in front of it, like we do for descendants.
def subclasses
if Vmdb::Application.instance.initialized? && !defined? @loaded_descendants
@loaded_descendants = true
DescendantLoader.instance.load_subclasses(self)
end

super
end
end

module AsDependenciesClearWithLoader
def clear
DescendantLoader.instance.clear_class_inheritance_relationships
Expand All @@ -292,12 +273,6 @@ def clear
end
end

# Patch Class to support non-AR models in the models directory
Class.prepend(DescendantLoader::ArDescendantsWithLoader)
# Patch ActiveRecord specifically to get ahead of ActiveSupport::DescendantsTracker
# The patching of Class does not put it in the right place in the ancestors chain
ActiveRecord::Base.singleton_class.prepend(DescendantLoader::ArDescendantsWithLoader)

at_exit do
DescendantLoader.instance.save_cache!
end