This project is archived and is in readonly mode.

#3788 ✓resolved
Paul Rosania

[PATCH] Rails 3.0.pre initializers do not always bind in the proper order

Reported by Paul Rosania | January 26th, 2010 @ 03:04 AM | in 3.0.2

Rails::Initializable::Collection does not always properly account for :after and :before requirements.

The sorting algorithm in Collection#initialize searches for the initializer specified by :after or :before, which works great if the initializer is already present. However, attempting to hook an initializer to another initializer that hasn't yet been loaded causes the find to fail, which puts the initializer at the end.

In order to handle dependencies at insertion time (as currently designed), one could envision a system that creates placeholders for these requirements.

Unfortunately I don't time to write a patch yet, so I'm just reporting the issue.

Comments and changes to this ticket

  • Paul Rosania

    Paul Rosania January 27th, 2010 @ 06:30 AM

    Patch attached for discussion.

    I have modified the dependency resolution algorithm to use topographical sort (leveraging tsort from the Ruby standard library).

    Following the principle of least surprise, and to satisfy pre-existing tests, initializer declarations now implicitly create an :after dependency on the prior initializer in the class (if there is one, and no explicit :after is stated).

    However, because of the nature of the dependency graph created during boot (DAG), it is not guaranteed that an initializer with an :after (s/after/before) will run immediately after the specified initializer dependency. Therefore, it is necessary in these rare cases to be explicit in stating both a :before and :after condition to make sure execution happens as desired. I have updated the tests to reflect this situation.

  • Paul Rosania

    Paul Rosania January 27th, 2010 @ 01:41 PM

    • Title changed from “Rails 3.0.pre initializers do not always bind in the proper order” to “[PATCH] Rails 3.0.pre initializers do not always bind in the proper order”
  • Jeremy Kemper

    Jeremy Kemper January 28th, 2010 @ 04:14 PM

    • State changed from “new” to “open”
    • Assigned user set to “Yehuda Katz (wycats)”
    • Milestone cleared.

    Good thinking, Paul! I get a test failure though:

      1) Failure:
    test_database_middleware_initializes_when_session_store_is_active_record(ApplicationTests::FrameworlsTest) [test/application/initializers/frameworks_test.rb:71]:
    <[ActiveRecord::ConnectionAdapters::ConnectionManagement,
     ActiveRecord::QueryCache,
     ActiveRecord::SessionStore]> expected but was
    <[ActiveRecord::SessionStore,
     ActiveRecord::ConnectionAdapters::ConnectionManagement,
     ActiveRecord::QueryCache]>.
    
  • Paul Rosania

    Paul Rosania January 28th, 2010 @ 04:25 PM

    Hmm. This is a side effect of the sort. Any test checking whether initializers loaded or not will need to be agnostic to the exact order. I will take a look and revise the test accordingly.

  • Paul Rosania

    Paul Rosania January 28th, 2010 @ 09:56 PM

    Updated patch attached.

    The new initializer method highlights an implicit :before dependency in the AR initializers. I made the dependency explicit, which resolves the outstanding test case.

    I should note that while the tsort method is much more robust, it necessarily increases the chances that implicit dependencies will break. Primarily, this impacts how one thinks about what a :before dependencies means.

    For example, under the proposed system, :after dependencies guarantee that the referenced initializer has been run. They do not guarantee that your initializer will run immediately afterward; in fact it could run at any point onward. A :before dependency is required if you want to box execution very specifically.

  • Anuj Dutta

    Anuj Dutta January 29th, 2010 @ 12:59 AM

    • Tag cleared.

    Hey Paul,

    Got this Error:

    test_plugin_init_is_ran_before_application_ones(RailtiesTest::PluginTest):
    RuntimeError: no $foo
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/tmp/app/config/initializers/foo.rb:1:in `'
        /Users/andhapp/Projects/git-repository/andhapp-rails/activesupport/lib/active_support/dependencies.rb:156:in `load'
        /Users/andhapp/Projects/git-repository/andhapp-rails/activesupport/lib/active_support/dependencies.rb:156:in `block in load_with_new_constant_marking'
        /Users/andhapp/Projects/git-repository/andhapp-rails/activesupport/lib/active_support/dependencies.rb:537:in `new_constants_in'
        /Users/andhapp/Projects/git-repository/andhapp-rails/activesupport/lib/active_support/dependencies.rb:156:in `load_with_new_constant_marking'
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/lib/rails/engine.rb:107:in `block (2 levels) in '
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/lib/rails/engine.rb:106:in `each'
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/lib/rails/engine.rb:106:in `block in '
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/lib/rails/initializable.rb:25:in `instance_exec'
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/lib/rails/initializable.rb:25:in `run'
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/lib/rails/initializable.rb:55:in `block in run_initializers'
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/lib/rails/initializable.rb:54:in `each'
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/lib/rails/initializable.rb:54:in `run_initializers'
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/lib/rails/application.rb:71:in `initialize!'
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/lib/rails/application.rb:41:in `method_missing'
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/tmp/app/config/environment.rb:5:in `'
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/test/railties/shared_tests.rb:6:in `require'
        /Users/andhapp/Projects/git-repository/andhapp-rails/railties/test/railties/shared_tests.rb:6:in `boot_rails'
        test/railties/plugin_test.rb:60:in `block in '
    
    
  • Paul Rosania

    Paul Rosania January 29th, 2010 @ 02:29 AM

    Okay. Here is one that definitely clears all Railties tests. Sorry for the misfire!

  • Anuj Dutta

    Anuj Dutta January 29th, 2010 @ 03:16 AM

    +1 Patch applies cleanly and all railties test pass.

  • José Valim

    José Valim February 23rd, 2010 @ 09:40 PM

    • State changed from “open” to “resolved”
    • Assigned user changed from “Yehuda Katz (wycats)” to “José Valim”

    I guess this one was already applied. Please tell me if I'm wrong.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) March 16th, 2010 @ 10:42 PM

    FWIW, I've found it possible to do a stable topological sort using tsort by yielding to the nodes in the order they were added. So if two nodes are equivalent, the original order will be preserved.

    I had track what order things were inserted in and sort by this order in tsort_each_node and tsort_each_child, but otherwise it was relatively simple change.

    Of course the extra sort is slightly less efficient than a pure tsort, I think that it matches most people's mental model of how dependencies should be resolved (i.e. things given equivalent weight in the sort would be processed in the order they were added).

  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to 3.0.2
    • Importance changed from “” to “Medium”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages