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 for nested require_dependencies #2193

Merged
merged 2 commits into from Nov 5, 2018

Conversation

Projects
None yet
4 participants
@genkami
Copy link
Contributor

genkami commented Oct 25, 2018

There are problems with nested require_dependencies.

Minimal example that reproduces this bug is: padrino-nested-require.tar.gz

This is how I got this bug:

$ bundle install
$ bundle exec ruby toplevel.rb
  ERROR -  NameError - uninitialized constant ALib:
...
Traceback (most recent call last):
toplevel.rb:10:in `<main>': uninitialized constant ModelWithNoDepOnLib (NameError)
Did you mean?  ModelThatDependsOnLib

Here is how this bug occurs:

  • A file toplevel.rb calls require_dependencies to load lib/a_lib.rb, lib/dependent_lib.rb, an_app/app.rb
  • The first loading of lib/a_lib.rb fails because it depends on lib/dependent_lib.rb
  • lib/dependent_lib.rb are loaded with no error because it does not depend on any other files.
  • Then require_dependencies tries to load an_app/app.rb, which may call nested require_dependencies.
  • Nested require_dependencies loads an_app/models/model_with_no_dep_on_lib.rb with no error.
  • Then nested require_dependencies tries to load an_app/models/model_that_depends_on_lib.rb, but it fails because model_that_depends_on_lib.rb depends on lib/a_lib.rb.
  • After all, nested require_dependencies gives up to load files, raising NameError - uninitialized constant ALib
  • Outer require_dependencies catches this exception and it assumes that entire process of loading an_app/app.rb fails, although model_with_no_dep_on_lib.rb was loaded and commited successfully.
  • Outer require_dependencies rollbacks every constants that is created before preparing an_app/app.rb
  • As a result, all constants defined in an_app/models/model_with_no_dep_on_lib.rb are removed, even though they are commited and not to be loaded in next nested require_dependencies.

Modify Gemfile in the example above as follows:

gem "padrino", github: "genkami/padrino-framework", ref: "b5186fe3"

Then execute bundle exec ruby toplevel.rb again. No error may happen.

@genkami genkami force-pushed the genkami:fix-nested-require-dependencies branch from 5a17cef to 221aee5 Nov 5, 2018

@genkami genkami force-pushed the genkami:fix-nested-require-dependencies branch from 41bb30f to b5186fe Nov 5, 2018

@namusyaka
Copy link
Member

namusyaka left a comment

Code change looks good, but I'm not familiar with the reloader component.
I'd like @ujifgc to review this for checking the validity.

@namusyaka namusyaka requested a review from ujifgc Nov 5, 2018

@ujifgc

ujifgc approved these changes Nov 5, 2018

Copy link
Member

ujifgc left a comment

Looks fine to me.

@namusyaka namusyaka merged commit 09103ef into padrino:master Nov 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

hirocaster added a commit to hirocaster/padrino-framework that referenced this pull request Nov 5, 2018

Fix incomplete Storage.rollback
Incomplete rollback at load
`Padrino.root("fixtures/dependencies/nested/l.rb")` first time. by padrino#2193

The real problem is not to require the rollbacked files again.
This happens because rollback does not delete file path in `Reloader.MTIME`.

ujifgc added a commit that referenced this pull request Nov 5, 2018

Fix incomplete Storage.rollback
Incomplete rollback at load
`Padrino.root("fixtures/dependencies/nested/l.rb")` first time. by #2193

The real problem is not to require the rollbacked files again.
This happens because rollback does not delete file path in `Reloader.MTIME`.

ujifgc added a commit that referenced this pull request Nov 6, 2018

@ujifgc

This comment has been minimized.

Copy link
Member

ujifgc commented Nov 6, 2018

Hey @genkami, thanks for your PR and thorough investigation.

In 338111d I changed the original code to fix your test exactly.

Can you think of another test that would break current implementation?

Related to #2197 #2196

@hirocaster hirocaster referenced this pull request Nov 6, 2018

Closed

Fix typo #2196 #2197

@hirocaster

This comment has been minimized.

Copy link
Contributor

hirocaster commented Nov 6, 2018

added tests(case) #2199

@genkami

This comment has been minimized.

Copy link
Contributor Author

genkami commented Nov 7, 2018

Looks good to me!

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