Guard against hitting const_missing from within const_missing #415

jrafanie · 2020-02-03T19:40:00Z

If MiqAeServiceModelBase is not yet defined, we could hit const_missing
recursively, leading to a stack level too deep.

Fixes ManageIQ/manageiq#19618

If MiqAeServiceModelBase is not yet defined, we could hit const_missing recursively, leading to a stack level too deep. Fixes ManageIQ/manageiq#19618

miq-bot · 2020-02-03T19:47:08Z

Checked commit jrafanie@3f4ea2c with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

coveralls · 2020-02-03T19:53:21Z

Pull Request Test Coverage Report for Build 3654

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.002%) to 85.453%

Totals
Change from base Build 3638:	0.002%
Covered Lines:	5040
Relevant Lines:	5898

💛 - Coveralls

gmcculloug · 2020-02-03T19:54:34Z

Thanks @jrafanie, this is a constant problem. (I learned from watching you.)

skateman

jrafanie · 2020-02-03T21:05:27Z

@gmcculloug it's green 😉

d-m-u · 2020-02-03T21:29:11Z

What, @gmcculloug, you're gonna let him get away with no tests?
/s

jrafanie · 2020-02-04T14:40:10Z

lib/miq_automation_engine/engine/miq_ae_method_service.rb

@@ -1,5 +1,7 @@
 module MiqAeMethodService
  def self.const_missing(name)
+    super unless defined?(MiqAeServiceModelBase)


@gmcculloug should this have been:

return super unless defined?(MiqAeServiceModelBase)

As is it now, it's possible to super twice.

or possibly more surgical:

return super if name.to_s.ends_with?("MiqAeServiceModelBase")

cc @d-m-u

1st one - no changes
2nd one:

NoMethodError: undefined method `create_service_model_from_name' for MiqAeMethodService::MiqAeServiceModelBase:Class

Error caught: [NoMethodError] undefined method `create_service_model_from_name' for MiqAeMethodService::MiqAeServiceModelBase:Class /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/bundler/gems/manageiq-automation_engine-358772c1409b/lib/miq_automation_engine/engine/miq_ae_method_service.rb:5:in `const_missing' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/bundler/gems/manageiq-automation_engine-358772c1409b/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:12:in `<class:MiqAeServiceModelBase>' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/bundler/gems/manageiq-automation_engine-358772c1409b/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:4:in `<module:MiqAeMethodService>' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/bundler/gems/manageiq-automation_engine-358772c1409b/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:3:in `<top (required)>' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:477:in `load' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:477:in `block in load_file' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:662:in `new_constants_in' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:476:in `load_file' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:374:in `block in require_or_load' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:36:in `block in load_interlock' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-5.1.7/lib/active_support/dependencies/interlock.rb:12:in `block in loading' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-5.1.7/lib/active_support/concurrency/share_lock.rb:149:in `exclusive' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-5.1.7/lib/active_support/dependencies/interlock.rb:11:in `loading' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:36:in `load_interlock' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:357:in `require_or_load' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:511:in `load_missing_constant' /home/skateman/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:202:in `const_missing'

I see, it's failing when reloading MiqAeServiceModelBase when trying to autoload the include MiqAeMethodService::MiqAeServiceObjectCommon line, so the constant exists but the method doesn't yet.

Maybe something like this? @skateman

diff --git a/lib/miq_automation_engine/engine/miq_ae_method_service.rb b/lib/miq_automation_engine/engine/miq_ae_method_service.rb index b0f668a..73eccd1 100644 --- a/lib/miq_automation_engine/engine/miq_ae_method_service.rb +++ b/lib/miq_automation_engine/engine/miq_ae_method_service.rb @@ -1,8 +1,10 @@ module MiqAeMethodService def self.const_missing(name) - super unless defined?(MiqAeServiceModelBase) - - MiqAeServiceModelBase.create_service_model_from_name(name) || super + if !defined?(MiqAeServiceModelBase) || !MiqAeServiceModelBase.respond_to?(:create_service_model_from_name) + super + else + MiqAeServiceModelBase.create_service_model_from_name(name) || super + end end end

yuck by the way

or a more positive way to write the code above

diff --git a/lib/miq_automation_engine/engine/miq_ae_method_service.rb b/lib/miq_automation_engine/engine/miq_ae_method_service.rb index b0f668a..50f8e71 100644 --- a/lib/miq_automation_engine/engine/miq_ae_method_service.rb +++ b/lib/miq_automation_engine/engine/miq_ae_method_service.rb @@ -1,8 +1,10 @@ module MiqAeMethodService def self.const_missing(name) - super unless defined?(MiqAeServiceModelBase) - - MiqAeServiceModelBase.create_service_model_from_name(name) || super + if defined?(MiqAeServiceModelBase) && MiqAeServiceModelBase.respond_to?(:create_service_model_from_name) + MiqAeServiceModelBase.create_service_model_from_name(name) || super + else + super + end end end

Nope, same problem 😕 let's get you a reproducer:

you need to have a catalog with a service dialog assigned that contains a dynamic field

you try to order the catalog and you see that the automation works

you force a code reload, ideally in the backend or the API

you try to order the catalog again and the automation won't return anything

Harpreet's MBU DB is a good starting point, there are multiple services there which you can use to test, will send you a link.

Files defining constants to be autoloaded should never be required ... Just follow the flow and use constant autoloading always, never mix autoloading and require. As a last resort, if some file absolutely needs to load a certain file use require_dependency to play nice with constant autoloading. This option is rarely needed in practice, though. Of course, using require in autoloaded files to load ordinary 3rd party libraries is fine, and Rails is able to distinguish their constants, they are not marked as autoloaded.

From: https://guides.rubyonrails.org/autoloading_and_reloading_constants_classic_mode.html#autoloading-and-require

I'm thinking it's because automation_engine is not rails code reloading friendly. I think it needs to autoload constants or use require_dependency so rails can track the constants loaded. See below, we're using require_relative instead of autoload/require_dependency so rails doesn't know about the constants. 🤷‍♂

I don't think this ever worked with code reloading cc @gmcculloug

lib/miq_automation_engine/engine/miq_ae_engine.rb: require_relative "miq_ae_engine/#{File.basename(file)}" lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_object.rb:require_relative 'miq_ae_state_machine' lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_workspace_runtime.rb:require_relative 'miq_ae_state_info' lib/miq_automation_engine/engine/miq_ae_method_service.rb: require_relative "miq_ae_method_service/#{File.basename(file)}" lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service.rb:require_relative './miq_ae_service_model_legacy' lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service.rb:require_relative './miq_ae_service_vmdb' lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service.rb:require_relative './miq_ae_service_rbac' lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:require_relative 'miq_ae_service_object_common' lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:require_relative 'miq_ae_service_rbac' lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_object.rb:require_relative './miq_ae_service_object_common' lib/miq_automation_engine/service_models/miq_ae_service_cloud_tenant.rb: require_relative "mixins/miq_ae_external_url_mixin" lib/miq_automation_engine/service_models/miq_ae_service_cloud_volume.rb: require_relative "mixins/miq_ae_service_ems_operations_mixin" lib/miq_automation_engine/service_models/miq_ae_service_ems_cluster.rb: require_relative "mixins/miq_ae_service_ems_operations_mixin" lib/miq_automation_engine/service_models/miq_ae_service_ems_folder.rb: require_relative "mixins/miq_ae_service_ems_operations_mixin" lib/miq_automation_engine/service_models/miq_ae_service_ext_management_system.rb: require_relative "mixins/miq_ae_service_inflector_mixin" lib/miq_automation_engine/service_models/miq_ae_service_ext_management_system.rb: require_relative "mixins/miq_ae_service_custom_attribute_mixin" lib/miq_automation_engine/service_models/miq_ae_service_ext_management_system.rb: require_relative "mixins/miq_ae_external_url_mixin" lib/miq_automation_engine/service_models/miq_ae_service_generic_object.rb: require_relative "mixins/miq_ae_service_remove_from_vmdb_mixin" lib/miq_automation_engine/service_models/miq_ae_service_generic_object.rb: require_relative "mixins/miq_ae_external_url_mixin" lib/miq_automation_engine/service_models/miq_ae_service_host.rb: require_relative "mixins/miq_ae_service_custom_attribute_mixin" lib/miq_automation_engine/service_models/miq_ae_service_host.rb: require_relative "mixins/miq_ae_service_ems_operations_mixin" lib/miq_automation_engine/service_models/miq_ae_service_host.rb: require_relative "mixins/miq_ae_service_remove_from_vmdb_mixin" lib/miq_automation_engine/service_models/miq_ae_service_load_balancer.rb: require_relative "mixins/miq_ae_service_retirement_mixin" lib/miq_automation_engine/service_models/miq_ae_service_load_balancer.rb: require_relative "mixins/miq_ae_service_remove_from_vmdb_mixin" lib/miq_automation_engine/service_models/miq_ae_service_manageiq-providers-vmware-infra_manager-host_esx.rb: require_relative "mixins/miq_ae_service_retirement_mixin" lib/miq_automation_engine/service_models/miq_ae_service_miq_group.rb: require_relative "mixins/miq_ae_service_custom_attribute_mixin" lib/miq_automation_engine/service_models/miq_ae_service_miq_group.rb: require_relative "mixins/miq_ae_external_url_mixin" lib/miq_automation_engine/service_models/miq_ae_service_miq_provision.rb: require_relative "mixins/miq_ae_service_miq_provision_mixin" lib/miq_automation_engine/service_models/miq_ae_service_miq_provision_request.rb: require_relative "mixins/miq_ae_service_miq_provision_mixin" lib/miq_automation_engine/service_models/miq_ae_service_miq_request.rb: require_relative "mixins/miq_ae_service_miq_request_mixin" lib/miq_automation_engine/service_models/miq_ae_service_miq_request_task.rb: require_relative "mixins/miq_ae_service_miq_request_mixin" lib/miq_automation_engine/service_models/miq_ae_service_orchestration_stack.rb: require_relative "mixins/miq_ae_service_retirement_mixin" lib/miq_automation_engine/service_models/miq_ae_service_orchestration_stack.rb: require_relative "mixins/miq_ae_service_remove_from_vmdb_mixin" lib/miq_automation_engine/service_models/miq_ae_service_service.rb: require_relative "mixins/miq_ae_service_retirement_mixin" lib/miq_automation_engine/service_models/miq_ae_service_service.rb: require_relative "mixins/miq_ae_service_custom_attribute_mixin" lib/miq_automation_engine/service_models/miq_ae_service_service.rb: require_relative "mixins/miq_ae_service_remove_from_vmdb_mixin" lib/miq_automation_engine/service_models/miq_ae_service_service.rb: require_relative "mixins/miq_ae_external_url_mixin" lib/miq_automation_engine/service_models/miq_ae_service_service_ansible_tower.rb: require_relative "mixins/miq_ae_service_service_ansible_tower_mixin" lib/miq_automation_engine/service_models/miq_ae_service_service_orchestration.rb: require_relative "mixins/miq_ae_service_service_orchestration_mixin" lib/miq_automation_engine/service_models/miq_ae_service_service_template_ansible_tower.rb: require_relative "mixins/miq_ae_service_service_ansible_tower_mixin" lib/miq_automation_engine/service_models/miq_ae_service_service_template_orchestration.rb: require_relative "mixins/miq_ae_service_service_orchestration_mixin" lib/miq_automation_engine/service_models/miq_ae_service_service_template_provision_request.rb: require_relative "mixins/miq_ae_service_miq_provision_mixin" lib/miq_automation_engine/service_models/miq_ae_service_tenant.rb: require_relative "mixins/miq_ae_external_url_mixin" lib/miq_automation_engine/service_models/miq_ae_service_user.rb: require_relative "mixins/miq_ae_service_custom_attribute_mixin" lib/miq_automation_engine/service_models/miq_ae_service_user.rb: require_relative "mixins/miq_ae_external_url_mixin" lib/miq_automation_engine/service_models/miq_ae_service_vm.rb: require_relative "mixins/miq_ae_external_url_mixin" lib/miq_automation_engine/service_models/miq_ae_service_vm_cloud_reconfigure_request.rb: require_relative "mixins/miq_ae_service_miq_provision_quota_mixin" lib/miq_automation_engine/service_models/miq_ae_service_vm_migrate_request.rb: require_relative "mixins/miq_ae_service_miq_provision_quota_mixin" lib/miq_automation_engine/service_models/miq_ae_service_vm_or_template.rb: require_relative "mixins/miq_ae_service_ems_operations_mixin" lib/miq_automation_engine/service_models/miq_ae_service_vm_or_template.rb: require_relative "mixins/miq_ae_service_retirement_mixin" lib/miq_automation_engine/service_models/miq_ae_service_vm_or_template.rb: require_relative "mixins/miq_ae_service_inflector_mixin" lib/miq_automation_engine/service_models/miq_ae_service_vm_or_template.rb: require_relative "mixins/miq_ae_service_custom_attribute_mixin" lib/miq_automation_engine/service_models/miq_ae_service_vm_or_template.rb: require_relative "mixins/miq_ae_service_remove_from_vmdb_mixin" lib/miq_automation_engine/service_models/miq_ae_service_vm_reconfigure_request.rb: require_relative "mixins/miq_ae_service_miq_provision_quota_mixin"

@jrafanie So your recommendation is to update these 50+ require_relative references to use require_dependency? Is that the only update required? Is there any other potential benefit other than supporting reload when running in dev mode?

Guard against hitting const_missing from within const_missing

3f4ea2c

If MiqAeServiceModelBase is not yet defined, we could hit const_missing recursively, leading to a stack level too deep. Fixes ManageIQ/manageiq#19618

jrafanie requested review from gmcculloug, lfu and mkanoor as code owners February 3, 2020 19:40

skateman approved these changes Feb 3, 2020

View reviewed changes

tinaafitz approved these changes Feb 3, 2020

View reviewed changes

mkanoor approved these changes Feb 3, 2020

View reviewed changes

gmcculloug self-assigned this Feb 3, 2020

gmcculloug added bug changelog/yes labels Feb 3, 2020

gmcculloug merged commit 358772c into ManageIQ:master Feb 3, 2020

gmcculloug added this to the Sprint 129 Ending Feb 3, 2020 milestone Feb 3, 2020

jrafanie deleted the guard_against_hitting_const_missing_within_const_missing branch February 3, 2020 22:04

jrafanie commented Feb 4, 2020

View reviewed changes

gmcculloug mentioned this pull request Feb 5, 2020

Investigate replacing require_relative with require_dependency #418

Open

jrafanie mentioned this pull request Sep 21, 2020

Overview of code loading issues leading to errors or deadlocks with long running http(s) requests ManageIQ/manageiq#20576

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guard against hitting const_missing from within const_missing #415

Guard against hitting const_missing from within const_missing #415

jrafanie commented Feb 3, 2020

miq-bot commented Feb 3, 2020

coveralls commented Feb 3, 2020 •

edited

gmcculloug commented Feb 3, 2020

skateman left a comment

jrafanie commented Feb 3, 2020

d-m-u commented Feb 3, 2020

jrafanie Feb 4, 2020

skateman Feb 4, 2020

skateman Feb 4, 2020

jrafanie Feb 4, 2020

jrafanie Feb 4, 2020

jrafanie Feb 4, 2020

skateman Feb 4, 2020

jrafanie Feb 4, 2020

gmcculloug Feb 5, 2020

Guard against hitting const_missing from within const_missing #415

Guard against hitting const_missing from within const_missing #415

Conversation

jrafanie commented Feb 3, 2020

miq-bot commented Feb 3, 2020

coveralls commented Feb 3, 2020 • edited

Pull Request Test Coverage Report for Build 3654

💛 - Coveralls

gmcculloug commented Feb 3, 2020

skateman left a comment

Choose a reason for hiding this comment

jrafanie commented Feb 3, 2020

d-m-u commented Feb 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 3, 2020 •

edited