Defining a module that fails to load, makes other modules that include it not have methods

[jjrussell]

Module A includes module B which includes module C

Module A
  include B
end

Module B
  include C
end

Module C ; end

When start none are defined. Files are required in sorted order A,B then C

1. A is required and tries to include B which raises a NameError. The module namespace A is now defined even though the include failed. It will be tried again later
2. B is required and tries to include C which raises a NameError. The module namespace B is now defined even though the include failed. It will be tried again later.
3. C is required and loads cleanly because it has no dependencies.
4. A is required again because it previously failed. It includes B which is now defined as a namespace but has NO METHODS in it because it failed to fully load before. Or at least not the ones from C which it should have since it includes C. So A included B successfully and did not get the methods from C but it worked.

A now does not have the methods you would expect and everything loaded cleanly.

I now understand the problem but I'm having a hard time coming up with a not awful solution to it.

Any thoughts?

[jarmo]

Can you change your example to have some method(s) defined as well so it's possible to reproduce that problem fully?

[jjrussell]

In an empty directory create three files

# a.rb
module A
  puts "A about to include B"
  include ::B
  puts "A finished including B"
end

# b.rb
module B
  puts "B about to include C"
  include ::C
  puts "B finished including C"
end

module C
  def c_method
    puts "I am C"
  end
end

And then a fourth file called run.rb

# run.rb
require 'require_all'
puts "Calling require_all on PWD"
$: << '.'
require 'c'
require 'b'
require 'a'

#require_all '.'
puts "Done with require_all"
puts "A has instance methods which are #{A.instance_methods}"

Initially we'll just do it with require in reverse order from C->A and the output of this will be

Calling require_all on PWD
B about to include C
B finished including C
A about to include B
A finished including B
Done with require_all
A has instance methods which are [:c_method]

Here we see that A when all is said and done has a method on it called c_method because it included B which includes C which has the method.

However if we change run.rb to comment out the require statements and go with require_all '.' the output is now

Calling require_all on PWD
A about to include B
B about to include C
Calling require_all on PWD
A about to include B
A finished including B
B about to include C
B finished including C
Done with require_all
here
A has instance methods which are []
Done with require_all
here
A has instance methods which are []

Not totally sure why it runs it twice but I think that it's require_all retrying after a LoadError or something. But at the end A does not have the method defined on C.

[jjrussell]

I purposely named the files a, b, and c to show that require_all does this when the files are sorted because it sorts the file list before requiring. This isn't bad but there is a chance this will all work if the file names are sorted the other way. For example if A was named C and visa versa this all works because A has the method and is loaded first.

[enkessler]

If we had a handy way of knowing what constants were defined before we attempted to load a file and what constants are defined after the attempted file load, we could remove from memory any new constants if a file fails to load cleanly, yes?

[jarmo]

Probably. I guess we should start by creating failing test and then trying
to fix the problem. I'm wondering if there's going to be a big performance
hit when it comes to bigger projects.

Anyway, it's worth to try.

On Tue, Oct 27, 2015 at 1:00 AM, Eric Kessler notifications@github.com
wrote:

If we had a handy way of knowing what constants were defined before we
attempted to load a file and what constants are defined after the attempted
file load, we could remove from memory any new constants if a file fails to
load cleanly, yes?

—
Reply to this email directly or view it on GitHub
#8 (comment).

[enkessler]

There's a decent chance of a performance hit, yes, but I would think that the most common use case of this gem are one time calls at startup. I mean, this gem is kind of a 'here's a pile of stuff, please figure it out for me' sort of thing. So we are theoretically dealing with users who aren't super reliant on performance or else they would already be taking the extra care of managing their file requiring on their own.

[jarmo]

Waiting for a possible fix/pull request. I do not probably have time to do that so if there's someone willing, I'd happily accept a pull request. Thanks!

[enkessler]

I just ran into this myself. Luckily I recall this issue existing, so I didn't waste too much time determining what was wrong and adding at least a little order to how I pull in files. Still, it would be nice if we could get this solved.

[jarmo]

Still waiting for a PR. Closing this due to inactivity until then.

[joehorsnell]

This is exactly the sort of issue that is difficult/impossible to fix given the way require_all currently works, so is why I opened the strict mode PR, to allow the simple addition of any explicit requires needed to ensure dependencies are loaded.

Regarding the earlier suggestion in this issue by @enkessler, ie of tracking which constants are defined before each file is required and unloading newly-added ones in the case of errors, I can see at least three very-hard-to-solve/unsolvable issues with that approach:

• Defining of classes/constants is not the only side-effect of requiring a file - eg what about methods or other state being added to other classes? Or expensive operations that must only happen once, the results of which are memoized in (for example) the singleton class of a class that's defined during the require? It might not be safe to do those operations again
• Indirectly-required constants - what about constants that were loaded as a side-effect of require_all requiring the file? ie it would be impossible to tell the difference between constants defined (directly) by the file just required and ones defined in files that were required by the file just required. You'd have to unload (and subsequently reload) all of them
• Very expensive - as pointed out above, it would be very expensive to get a list of the (global) set of loaded constants before each file and cause a fair amount of GC pressure as all those objects have to be collected, all for marginal benefit.

Thoughts on #21 @jarmo?