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

Fix remove_constant to stop removing constants that are already remove_constanted but not GCed #2200

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

genkami
Copy link
Contributor

@genkami genkami commented Nov 7, 2018

I found a bug with remove_constant and GC.

I wll later give an example that potentially causes this bug.

@namusyaka
Copy link
Contributor

@genkami Could you provide an example for reproducing the error?

@genkami
Copy link
Contributor Author

genkami commented Jul 12, 2019

I added a test case that reproduces the errror. It fails without 8542c62.

This is how this error occurs:

  • First, we require two files: t.rb, u.rb
  • t.rb requires v.rb, w.rb, x.rb with nested require_dependencies.
  • The first loading of v.rb fails and V is removed because it depends on w.rb. (we call this V V1)
  • Then, w.rb is loaded successfully, and it defines W.
  • Then, we try to load x.rb, but it fails because x.rb depends on y.rb
  • Then, we try to load v.rb again, and it succeeds. (we call the V defined here V2)
  • Eventually, we find that we can't load x.rb because we can't resolve its dependencies.
  • After all, the entire process of loading t.rb is considered as failure.
  • All constants that are defined in loading a.rb are going to be rollbacked, except for V2 and W. In this situation, V1 are considered as a constant that should be removed because it's defined in neither v.rb nor w.rb. So we tries to remove V1, but it results to removing V2 because V1.to_s == V2.to_s == "V".
  • After rollback, we load x.rb and its dependency (y.rb).
  • Then, we try to load t.rb again, but only x.rb are loaded as t.rb's dependencies, because we already loaded v.rb with no rollback.
  • As a result, V is never defined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants