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

Fix bug with eager_load in development environment #33118

Merged
merged 1 commit into from Jun 11, 2018

Conversation

Projects
None yet
3 participants
@dan-jensen
Contributor

dan-jensen commented Jun 11, 2018

Summary

Fixes the "Development Environment Lockup" bug reported in Issue #30578, and adds test coverage to protect against any similar bug in the future.

Problem

If config.eager_load = true in development, then any change to a model or controller will cause an ArgumentError (unknown firstpos: NilClass) on the next request.

The problem occurs when secondary Engines are loaded (the Rails application is the primary Engine). In the case of a default Rails 5.1/5.2 install, the secondary Engine triggering the problem is ActiveStorage - stop loading ActiveStorage and the problem goes away. This is because, for every secondary Engine, a new RouteSet is created. These secondary RouteSets are apparently not used – routes are not added to them (only to the primary RouteSet). In other words, when you have a secondary Engine, then you have an empty RouteSet. But ActionDispatch::Journey::Routes chokes on empty RouteSets. Specifically, the #simulator method expects the #asts method to return an instance of Nodes::Or, but when a RouteSet is empty the #asts method returns nil and triggers the exception.

This bug was apparently introduced in 5b1332b, which calls the routes simulator during the eager_load process.

Solution

I think routes simulation should not trigger an exception when there is an empty RouteSet. The most elegant solution seems to be removing the unless asts.empty? that was introduced in a refactor commit in 2013: 2467ec8#diff-d72add3debad9b63513514835f2a70b7R44. I'm reluctant to change something so well established, but I can't determine why it was added, and it seems to have been unnecessary. With this change, the #asts method returns a Nodes::Or object when the RouteSet is empty (with its @children variable simply set to an empty array). This results in the #simulator method receiving a Nodes::Or object as expected, thereby avoiding the ArgumentError exception.

Fix bug with eager_load in development environment
Modifies the routes simulator to allow for empty RouteSets, which are
created when secondary Engines are loaded.

@rafaelfranca rafaelfranca merged commit a7d394b into rails:master Jun 11, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
codeclimate All good!
Details

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Jun 11, 2018

Merge pull request rails#33118 from dan-jensen/fix-eager-load-lockup-…
…bug-in-development

Fix bug with eager_load in development environment

cherry-pick a7d394b

Fixes rails#30578.
Closes rails#32296.

kamipo added a commit that referenced this pull request Jun 11, 2018

Merge pull request #33118 from dan-jensen/fix-eager-load-lockup-bug-i…
…n-development

Fix bug with eager_load in development environment

cherry-pick a7d394b

Fixes #30578.
Closes #32296.
@matthewd

This comment has been minimized.

Member

matthewd commented Jun 12, 2018

For the record, the empty? check has been there (under different spellings) since ast was first introduced: in rails/journey@5e23bd1#diff-6b67d28f40aaad449c12eece8ba7d506R99 it was implicit in the inject.

rails/journey@242817a#diff-6b67d28f40aaad449c12eece8ba7d506R97 explicitly pulled it out as a conditional, so that's the most likely to hold clues about its intent.

Its effect, which this PR changes, is to leave @ast unset until there's at least one route present. Before rails/journey@c6c99da that might've mattered, but it seems unnecessary since then. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment