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

Rails falls back on non-thread-safe autoloading even when eager_load is true #13142

Closed
jnicklas opened this Issue Dec 3, 2013 · 30 comments

Comments

Projects
None yet
@jnicklas
Contributor

jnicklas commented Dec 3, 2013

In Rails 3.x, the disable_dependency_loading config option was available for controlling whether autoloading was enabled as a fallback in case eager loading fails. This option has been removed from Rails in this commit e6747d8. The behaviour was changed so that autoload is disabled entirely when eager loading is enabled. This seems to make sense to me.

However, in a8bf129, the initializer which disabled autoload when eager load is true was removed entirely.

Now we're stuck with Rails behaving in a non-threadsafe way by default, which is possibly very difficult to detect. I realize I can add my own initializer to solve this problem, but if we're encouraging people to build threadsafe apps by default now, then it shouldn't require this level of knowledge to make Rails behave in a thread-safe way.

The reasoning in a8bf129 seems flawed to me. Failing hard is a good thing, failing hard means subtle thread safety issues don't arise in a production environment sometimes because someone forgot a require somewhere. Thread safety is hard enough as it is.

In my opinion, a8bf129 should be reverted. If this is deemed too aggressive, then maybe the disable_dependency_loading option should be reinstated, probably set to true by default, I can prepare a pull request for this if this is the preferred option.

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva
Member

carlosantoniodasilva commented Dec 3, 2013

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 3, 2013

Member

I remember that one of the issues that made a8bf129 is that people usually put lib in the autoload path, and since lib usually may have a lot of development/test only files these files will be loaded by default in production.

In my opinion we should bring back disable_dependcy_loading

Member

rafaelfranca commented Dec 3, 2013

I remember that one of the issues that made a8bf129 is that people usually put lib in the autoload path, and since lib usually may have a lot of development/test only files these files will be loaded by default in production.

In my opinion we should bring back disable_dependcy_loading

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 3, 2013

Member

Between this was the biggest discussion in the history of the Rails core and seems we will start again 😄

Member

rafaelfranca commented Dec 3, 2013

Between this was the biggest discussion in the history of the Rails core and seems we will start again 😄

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 3, 2013

Member

@jnicklas unfortunately both reverting a8bf129 and adding back disable_dependency_loading are problematic because the only way to really test it is live in production.

However, Rails is still threadsafe by default because all the default autoload paths are eagerly loaded as well. The problem only appears when a developer adds a directory to config.autoload_paths that is not eagerly loaded - typically this is the lib folder. This folder often contains all sorts of junk and because of this it's not safe to eagerly load it by default as you would see strange errors when running in production by including development related code and libraries.

The alternative of hard failing is unpalatable as it's often old apps with weaker test coverage that have added lib to config.autoload_paths and making these fail hard is something we decided after a lengthy discussion that we didn't want to do (93 messages and multiple Campfire discussions!).

If developers follow our guidelines of not changing autoload_paths, adding directories to app for autoloading code and explicitly requiring code in lib then Rails will be threadsafe.

Member

pixeltrix commented Dec 3, 2013

@jnicklas unfortunately both reverting a8bf129 and adding back disable_dependency_loading are problematic because the only way to really test it is live in production.

However, Rails is still threadsafe by default because all the default autoload paths are eagerly loaded as well. The problem only appears when a developer adds a directory to config.autoload_paths that is not eagerly loaded - typically this is the lib folder. This folder often contains all sorts of junk and because of this it's not safe to eagerly load it by default as you would see strange errors when running in production by including development related code and libraries.

The alternative of hard failing is unpalatable as it's often old apps with weaker test coverage that have added lib to config.autoload_paths and making these fail hard is something we decided after a lengthy discussion that we didn't want to do (93 messages and multiple Campfire discussions!).

If developers follow our guidelines of not changing autoload_paths, adding directories to app for autoloading code and explicitly requiring code in lib then Rails will be threadsafe.

@pftg

This comment has been minimized.

Show comment
Hide comment
@pftg

pftg Dec 3, 2013

Contributor

Agree with @pixeltrix

Contributor

pftg commented Dec 3, 2013

Agree with @pixeltrix

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Dec 3, 2013

Contributor

@pixeltrix Where are these guidelines documented? If changing autoload_paths is discouraged, then this should be documented somewhere, as far as I can tell, no such documentation exists. Ideally, manipulating the list of load paths should generate a warning. I know this is common cannon, but "everyone knows this" really doesn't cut it in terms of documentation.

I'm worried by the fact that this whole reasoning builds on the fact that eager_load works perfectly, can we really guarantee that it does? With this in place, autoloading is still enabled, it could still be triggered, it could still create thread-safety problems. My experience with threaded programming is that you want to eliminate even the possibility of thread-safety issues if you can.

Contributor

jnicklas commented Dec 3, 2013

@pixeltrix Where are these guidelines documented? If changing autoload_paths is discouraged, then this should be documented somewhere, as far as I can tell, no such documentation exists. Ideally, manipulating the list of load paths should generate a warning. I know this is common cannon, but "everyone knows this" really doesn't cut it in terms of documentation.

I'm worried by the fact that this whole reasoning builds on the fact that eager_load works perfectly, can we really guarantee that it does? With this in place, autoloading is still enabled, it could still be triggered, it could still create thread-safety problems. My experience with threaded programming is that you want to eliminate even the possibility of thread-safety issues if you can.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 4, 2013

Member

@jnicklas I thought we had something in Configuring Rails Applications but I may have been thinking of Ryan Bigg's guides. If there's nothing, then I will add something.

Ideally, manipulating the list of load paths should generate a warning

We went through a list of possible scenarios about warning people - they all sucked in one way or another.

I'm worried by the fact that this whole reasoning builds on the fact that eager_load works perfectly, can we really guarantee that it does?

Does eager loading work perfectly? Probably not as nothing is perfect but I know that it'll require every file in your config.eager_load_paths array which is as good as it's likely to get. We can't protect against people doing crazy stuff like requiring the same file in different threads so something may break. Our default position can't be to break people's apps in production after they've upgraded by disabling all code loading, so given that I've not seem any significant traffic on this topic since we've released Rails 4 I'm loathe to make changes to it.

Member

pixeltrix commented Dec 4, 2013

@jnicklas I thought we had something in Configuring Rails Applications but I may have been thinking of Ryan Bigg's guides. If there's nothing, then I will add something.

Ideally, manipulating the list of load paths should generate a warning

We went through a list of possible scenarios about warning people - they all sucked in one way or another.

I'm worried by the fact that this whole reasoning builds on the fact that eager_load works perfectly, can we really guarantee that it does?

Does eager loading work perfectly? Probably not as nothing is perfect but I know that it'll require every file in your config.eager_load_paths array which is as good as it's likely to get. We can't protect against people doing crazy stuff like requiring the same file in different threads so something may break. Our default position can't be to break people's apps in production after they've upgraded by disabling all code loading, so given that I've not seem any significant traffic on this topic since we've released Rails 4 I'm loathe to make changes to it.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Dec 4, 2013

Contributor

The thing that worries me is not so much that it misses a file, but rather that it misses a constant. There any number of ways an additional, undefined constant might pop up during request processing. What happens then? Since autoloading is still enabled, we'll still hit load_missing_constant, this methods uses a lot of global state. I have no idea what'll happen, maybe it works just fine because there is no possibility of anything being mutated, but I don't know the code well enough.

Contributor

jnicklas commented Dec 4, 2013

The thing that worries me is not so much that it misses a file, but rather that it misses a constant. There any number of ways an additional, undefined constant might pop up during request processing. What happens then? Since autoloading is still enabled, we'll still hit load_missing_constant, this methods uses a lot of global state. I have no idea what'll happen, maybe it works just fine because there is no possibility of anything being mutated, but I don't know the code well enough.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Dec 4, 2013

Contributor

By the way, we ended up calling unhook! in an after_initialize block. IMO this is a good compromise, autoloading is enabled during the initialization process, which means all its convenience is there, and we don't have to worry too much about explicit requires. Since initialization is single-threaded this is safe. However, once we start processing requests, constants can no longer be autoloaded. Maybe I'm missing something, but this seems pretty safe to me.

Contributor

jnicklas commented Dec 4, 2013

By the way, we ended up calling unhook! in an after_initialize block. IMO this is a good compromise, autoloading is enabled during the initialization process, which means all its convenience is there, and we don't have to worry too much about explicit requires. Since initialization is single-threaded this is safe. However, once we start processing requests, constants can no longer be autoloaded. Maybe I'm missing something, but this seems pretty safe to me.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 4, 2013

Member

However, once we start processing requests, constants can no longer be autoloaded. Maybe I'm missing something, but this seems pretty safe to me.

The scenario is that a developer has added lib to config.autoload_path which is not eager loaded because it's full of junk you don't want in production like tasks, generators, etc. You then call unhook! and start processing requests but a controller action not yet requested refers to an undefined constant and when it's reached it'll blow up. It's this that we want to prevent when upgrading Rails applications.

Member

pixeltrix commented Dec 4, 2013

However, once we start processing requests, constants can no longer be autoloaded. Maybe I'm missing something, but this seems pretty safe to me.

The scenario is that a developer has added lib to config.autoload_path which is not eager loaded because it's full of junk you don't want in production like tasks, generators, etc. You then call unhook! and start processing requests but a controller action not yet requested refers to an undefined constant and when it's reached it'll blow up. It's this that we want to prevent when upgrading Rails applications.

@route

This comment has been minimized.

Show comment
Hide comment
@route

route Dec 4, 2013

Contributor

@pixeltrix what about adding app/lib directory to brand new rails application?

Contributor

route commented Dec 4, 2013

@pixeltrix what about adding app/lib directory to brand new rails application?

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 6, 2013

Member

what about adding app/lib directory to brand new rails application?

You can add it now if you want and it'll work like any other app folder - I don't think we want to add it by default as a lot of apps have empty lib folders so adding another empty folder seems unnecessary.

Member

pixeltrix commented Dec 6, 2013

what about adding app/lib directory to brand new rails application?

You can add it now if you want and it'll work like any other app folder - I don't think we want to add it by default as a lot of apps have empty lib folders so adding another empty folder seems unnecessary.

@pixeltrix pixeltrix closed this Dec 6, 2013

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Dec 6, 2013

Contributor

@pixeltrix you wrote this:

You then call unhook! and start processing requests but a controller action not yet requested refers to an undefined constant and when it's reached it'll blow up. It's this that we want to prevent when upgrading Rails applications.

The problem is that in the case where an app is multi-threaded, and we don't switch off autoload, the case would be that it probably won't blow up, but random stuff will mysteriously sometimes fail in weird ways. So ask yourself this, what would you rather want, option 1) where you can get an exception at runtime, or option 2) where you get random, unpredictable, weird, hard to explain, difficult to debug bugs at runtime.

Personally, I'm going to choose option 1. The downside of thread-safety issues is so much worse than the downside of the possibility of an exception. The way you're handling it makes it sound as though thread-safety is not important, as though Rails is still optimizing for the single-threaded case. That seems like a huge step back.

Contributor

jnicklas commented Dec 6, 2013

@pixeltrix you wrote this:

You then call unhook! and start processing requests but a controller action not yet requested refers to an undefined constant and when it's reached it'll blow up. It's this that we want to prevent when upgrading Rails applications.

The problem is that in the case where an app is multi-threaded, and we don't switch off autoload, the case would be that it probably won't blow up, but random stuff will mysteriously sometimes fail in weird ways. So ask yourself this, what would you rather want, option 1) where you can get an exception at runtime, or option 2) where you get random, unpredictable, weird, hard to explain, difficult to debug bugs at runtime.

Personally, I'm going to choose option 1. The downside of thread-safety issues is so much worse than the downside of the possibility of an exception. The way you're handling it makes it sound as though thread-safety is not important, as though Rails is still optimizing for the single-threaded case. That seems like a huge step back.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 8, 2013

Member

So ask yourself this, what would you rather want, option 1) where you can get an exception at runtime, or option 2) where you get random, unpredictable, weird, hard to explain, difficult to debug bugs at runtime.

Depends on the situation, would you prefer your highly traffic, e-commerce website to fail for all orders in the checkout or a small percentage of them. You may say option one but the accountant may say option 2.

Personally, I'm going to choose option 1. The downside of thread-safety issues is so much worse than the downside of the possibility of an exception. The way you're handling it makes it sound as though thread-safety is not important, as though Rails is still optimizing for the single-threaded case. That seems like a huge step back.

As long as the code you write is thread safe then you'll be fine. If it's not then whether we autoload or not isn't really going to save you if you don't know what you're doing as there are many ways to make code not thread safe - autoloading is just one of them. All Rails is doing is not 👊-ing you with an exceptions when you upgrade - if you want to 👊 yourself by all means go ahead. 😄

I'm more than willing to accept a patch to the guides telling developers how to remove the autoloading but I think we have the right default given the amount of thought that went into making the decision.

Member

pixeltrix commented Dec 8, 2013

So ask yourself this, what would you rather want, option 1) where you can get an exception at runtime, or option 2) where you get random, unpredictable, weird, hard to explain, difficult to debug bugs at runtime.

Depends on the situation, would you prefer your highly traffic, e-commerce website to fail for all orders in the checkout or a small percentage of them. You may say option one but the accountant may say option 2.

Personally, I'm going to choose option 1. The downside of thread-safety issues is so much worse than the downside of the possibility of an exception. The way you're handling it makes it sound as though thread-safety is not important, as though Rails is still optimizing for the single-threaded case. That seems like a huge step back.

As long as the code you write is thread safe then you'll be fine. If it's not then whether we autoload or not isn't really going to save you if you don't know what you're doing as there are many ways to make code not thread safe - autoloading is just one of them. All Rails is doing is not 👊-ing you with an exceptions when you upgrade - if you want to 👊 yourself by all means go ahead. 😄

I'm more than willing to accept a patch to the guides telling developers how to remove the autoloading but I think we have the right default given the amount of thought that went into making the decision.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Dec 8, 2013

Contributor

Who is going to run into a situation where all their orders fail and they don't notice? There are staging environments and smoke tests for these kinds of things. It's way more insidious and harder to notice when 0.1% of your orders fail as opposed to 100% of your orders failing. That's the point I'm trying to make. Failing hard is good. Especially with regards to thread safety.

Also, honestly, you're telling me to go fuck myself? What kind of way is that of handling legitimate issues in an open source project? I don't think I've been discourteous in any way, I'm simply trying to get you to see my perspective on this. In 8 years in the Ruby community, I've never seen this much discussed shitty attitude we supposedly have, until today. I'm seriously very disappointed.

Contributor

jnicklas commented Dec 8, 2013

Who is going to run into a situation where all their orders fail and they don't notice? There are staging environments and smoke tests for these kinds of things. It's way more insidious and harder to notice when 0.1% of your orders fail as opposed to 100% of your orders failing. That's the point I'm trying to make. Failing hard is good. Especially with regards to thread safety.

Also, honestly, you're telling me to go fuck myself? What kind of way is that of handling legitimate issues in an open source project? I don't think I've been discourteous in any way, I'm simply trying to get you to see my perspective on this. In 8 years in the Ruby community, I've never seen this much discussed shitty attitude we supposedly have, until today. I'm seriously very disappointed.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Dec 8, 2013

Member

Who is going to run into a situation where all their orders fail and they don't notice? There are staging environments and smoke tests for these kinds of things.

Trust me, not every project has these kind of things - in an ideal world, yes, but we don't live in an ideal world.

Also, honestly, you're telling me to go fuck myself?

I think you've misinterpreted what I was trying to say - what I'm saying is that going for a hard fail is an aggressive choice. That may be your preference but we can't adopt it as a default, however I'm willing to accept changes to make it easier if a developer wishes be that aggressive in their own projects. If I caused offence, I'm sorry.

Member

pixeltrix commented Dec 8, 2013

Who is going to run into a situation where all their orders fail and they don't notice? There are staging environments and smoke tests for these kinds of things.

Trust me, not every project has these kind of things - in an ideal world, yes, but we don't live in an ideal world.

Also, honestly, you're telling me to go fuck myself?

I think you've misinterpreted what I was trying to say - what I'm saying is that going for a hard fail is an aggressive choice. That may be your preference but we can't adopt it as a default, however I'm willing to accept changes to make it easier if a developer wishes be that aggressive in their own projects. If I caused offence, I'm sorry.

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Feb 16, 2015

Member

It's way more insidious and harder to notice when 0.1% of your orders fail as opposed to 100% of your orders failing.

I agree with this statement so much. We should absolutely be failing hard rather than forcing people to debug thread safety issues at runtime. I can't think of anything more infuriating than debugging an issue that happens "sometimes".

I'm reverting a8bf129. We can add a way to let you opt out, but allowing thread un-safe actions by default is not good.

Member

tenderlove commented Feb 16, 2015

It's way more insidious and harder to notice when 0.1% of your orders fail as opposed to 100% of your orders failing.

I agree with this statement so much. We should absolutely be failing hard rather than forcing people to debug thread safety issues at runtime. I can't think of anything more infuriating than debugging an issue that happens "sometimes".

I'm reverting a8bf129. We can add a way to let you opt out, but allowing thread un-safe actions by default is not good.

zzak added a commit to zzak/rails that referenced this issue Feb 16, 2015

Revert c732a66:
* "rm docs for dependency_loading and disable_dependency_loading config"

It was added back in a71350c by @tenderlove

See also #13142
@zdennis

This comment has been minimized.

Show comment
Hide comment
@zdennis

zdennis Apr 12, 2016

Contributor

Should any one stumble upon this issue @tenderlove reverted commit a8bf129 in a71350c which is in v5.0.0.beta1 and later.

Contributor

zdennis commented Apr 12, 2016

Should any one stumble upon this issue @tenderlove reverted commit a8bf129 in a71350c which is in v5.0.0.beta1 and later.

zdennis added a commit to Aperta-project/Aperta that referenced this issue Apr 12, 2016

Adding explicit require_dependency in app/**/*.rb files when they mak…
…e reference

to constants defined in lib/

This is to avoid threading issues with Rails auto-loading as described in rails/rails#13142

This issue has been resolved in Rails as of this commit: rails/rails@a71350c 

But that commit is not available until Rails v5.0.0.beta1 and later at the time of this commit.

APERTA-6537

zdennis added a commit to Aperta-project/Aperta that referenced this issue Apr 12, 2016

Adding explicit require_dependency in app/**/*.rb files when they mak…
…e reference

to constants defined in lib/

This is to avoid lack of thread-safety with Rails auto-loading as described in rails/rails#13142

This issue has been resolved in Rails as of this commit: rails/rails@a71350c

But that commit is not available until Rails v5.0.0.beta1 and later at the time of this commit.

NOTES FROM INITIAL INVESTIGATION:

This solution works because the app is eager_loaded and everything inside of the app/ directory is loaded when the application boots. This loads EventStream::Notifiable and in turn will load Notifier.

To see the issue in action you need to load "rails console" before this commit:

 * Edit lib/notifier.rb and put "sleep 5" right inside the "class Notifier" declaration
 * Open rails console
 * In console verify that Notifier is NOT loaded: Object.constants.map(&:to_s).grep /Notifier/
 * In console re-create issue:

      arr = []
      arr << Thread.new { puts Notifier }
      arr << Thread.new { puts Notifier }
      arr.map(&:join)

This is not related to the Notifier name. The constant could be called anything else and this issue would still fail.

It seems that Ruby (or Rails, but most likely Ruby) is internally keeping track of what constant is currently being auto-loaded. When the second thread attempts to auto-load the same constant (while the first thread hasn't finished loading it yet) the "Circular dependency" error is raised.

Paired with Sam Bleckley (@diiq) on figuring this out.

APERTA-6537

zdennis added a commit to Aperta-project/Aperta that referenced this issue Apr 12, 2016

Backported fix from Rails 5 to remove autoload threading issue.
Related to issue: rails/rails#13142
Fix pulled from: rails/rails@a71350c

This will disable autoloading when eager_loading=true and cache_classes=true
(typical in test and production environments) in the Rails.application.config.
This causes constants that are not loaded to fail.

Thanks Erik Hetzner (@egh) for the suggestion of backporting.

APERTA-6537

zdennis added a commit to Aperta-project/Aperta that referenced this issue Apr 12, 2016

Adding explicit require_dependency in app/**/*.rb files when they mak…
…e reference

to constants defined in lib/

This is to avoid lack of thread-safety with Rails auto-loading as described in rails/rails#13142

This issue has been resolved in Rails as of this commit: rails/rails@a71350c

But that commit is not available until Rails v5.0.0.beta1 and later at the time of this commit.

NOTES FROM INITIAL INVESTIGATION:

This solution works because the app is eager_loaded and everything inside of the app/ directory is loaded when the application boots. This loads EventStream::Notifiable and in turn will load Notifier.

To see the issue in action you need to load "rails console" before this commit:

 * Edit lib/notifier.rb and put "sleep 5" right inside the "class Notifier" declaration
 * Open rails console
 * In console verify that Notifier is NOT loaded: Object.constants.map(&:to_s).grep /Notifier/
 * In console re-create issue:

      arr = []
      arr << Thread.new { puts Notifier }
      arr << Thread.new { puts Notifier }
      arr.map(&:join)

This is not related to the Notifier name. The constant could be called anything else and this issue would still fail.

It seems that Ruby (or Rails, but most likely Ruby) is internally keeping track of what constant is currently being auto-loaded. When the second thread attempts to auto-load the same constant (while the first thread hasn't finished loading it yet) the "Circular dependency" error is raised.

Paired with Sam Bleckley (@diiq) on figuring this out.

APERTA-6537

zdennis added a commit to Aperta-project/Aperta that referenced this issue Apr 12, 2016

Backported fix from Rails 5 to remove autoload threading issue.
Related to issue: rails/rails#13142
Fix pulled from: rails/rails@a71350c

This will disable autoloading when eager_loading=true and cache_classes=true
(typical in test and production environments) in the Rails.application.config.
This causes constants that are not loaded to fail.

Thanks Erik Hetzner (@egh) for the suggestion of backporting.

APERTA-6537

zdennis added a commit to Aperta-project/Aperta that referenced this issue Apr 12, 2016

Adding explicit require_dependency in app/**/*.rb files when they mak…
…e reference

to constants defined in lib/

This is to avoid lack of thread-safety with Rails auto-loading as described in rails/rails#13142

This issue has been resolved in Rails as of this commit: rails/rails@a71350c

But that commit is not available until Rails v5.0.0.beta1 and later at the time of this commit.

NOTES FROM INITIAL INVESTIGATION:

This solution works because the app is eager_loaded and everything inside of the app/ directory is loaded when the application boots. This loads EventStream::Notifiable and in turn will load Notifier.

To see the issue in action you need to load "rails console" before this commit:

 * Edit lib/notifier.rb and put "sleep 5" right inside the "class Notifier" declaration
 * Open rails console
 * In console verify that Notifier is NOT loaded: Object.constants.map(&:to_s).grep /Notifier/
 * In console re-create issue:

      arr = []
      arr << Thread.new { puts Notifier }
      arr << Thread.new { puts Notifier }
      arr.map(&:join)

This is not related to the Notifier name. The constant could be called anything else and this issue would still fail.

It seems that Ruby (or Rails, but most likely Ruby) is internally keeping track of what constant is currently being auto-loaded. When the second thread attempts to auto-load the same constant (while the first thread hasn't finished loading it yet) the "Circular dependency" error is raised.

Paired with Sam Bleckley (@diiq) on figuring this out.

APERTA-6537

zdennis added a commit to Aperta-project/Aperta that referenced this issue Apr 14, 2016

Backported fix from Rails 5 to remove autoload threading issue.
Related to issue: rails/rails#13142
Fix pulled from: rails/rails@a71350c

This will disable autoloading when eager_loading=true and cache_classes=true
(typical in test and production environments) in the Rails.application.config.
This causes constants that are not loaded to fail.

Thanks Erik Hetzner (@egh) for the suggestion of backporting.

APERTA-6537

zdennis added a commit to Aperta-project/Aperta that referenced this issue Apr 14, 2016

Adding explicit require_dependency in app/**/*.rb files when they mak…
…e reference

to constants defined in lib/

This is to avoid lack of thread-safety with Rails auto-loading as described in rails/rails#13142

This issue has been resolved in Rails as of this commit: rails/rails@a71350c

But that commit is not available until Rails v5.0.0.beta1 and later at the time of this commit.

NOTES FROM INITIAL INVESTIGATION:

This solution works because the app is eager_loaded and everything inside of the app/ directory is loaded when the application boots. This loads EventStream::Notifiable and in turn will load Notifier.

To see the issue in action you need to load "rails console" before this commit:

 * Edit lib/notifier.rb and put "sleep 5" right inside the "class Notifier" declaration
 * Open rails console
 * In console verify that Notifier is NOT loaded: Object.constants.map(&:to_s).grep /Notifier/
 * In console re-create issue:

      arr = []
      arr << Thread.new { puts Notifier }
      arr << Thread.new { puts Notifier }
      arr.map(&:join)

This is not related to the Notifier name. The constant could be called anything else and this issue would still fail.

It seems that Ruby (or Rails, but most likely Ruby) is internally keeping track of what constant is currently being auto-loaded. When the second thread attempts to auto-load the same constant (while the first thread hasn't finished loading it yet) the "Circular dependency" error is raised.

Paired with Sam Bleckley (@diiq) on figuring this out.

APERTA-6537

jremmen added a commit to puffycheeks/store that referenced this issue Aug 19, 2016

vlymar pushed a commit to dobtco/fortitude that referenced this issue Aug 30, 2016

@vlymar vlymar referenced this issue Sep 13, 2016

Closed

Rails5 Support #51

3 of 3 tasks complete

vlymar pushed a commit to dobtco/fortitude that referenced this issue Sep 14, 2016

vlymar
Do not autoload after initialization.
Rails 5 by default disables autoloading after initialization in
production. There is one spec in the fortitude test suite that depends
on this autoloading behavior, but it tests a strange use case and we are
not supporting it.

The use case is the ability to reference a constant like
Views::SomeNamespace that corresponds to an empty directory
app/views/some_namespace/ . All actual widgets are eager loaded.

Autoloading after initialization is not thread safe. For more info on
why it was disabled, see rails/rails#13142
@akostadinov

This comment has been minimized.

Show comment
Hide comment
@akostadinov

akostadinov Dec 5, 2016

IMO auto-load should be disabled as a feature completely if it is not going to work. It is very frustrating for a new rail developer to read in documentation to add lib into auto_load list and then trying to deploy strange errors begin to happen.

And auto_load can happily start raising an error that it is not supported and eaget_load needs to be used. This will not confuse anybody unlike present situation.

It could be documented that lib can be added to eager_load instead which works fine in production env.

akostadinov commented Dec 5, 2016

IMO auto-load should be disabled as a feature completely if it is not going to work. It is very frustrating for a new rail developer to read in documentation to add lib into auto_load list and then trying to deploy strange errors begin to happen.

And auto_load can happily start raising an error that it is not supported and eaget_load needs to be used. This will not confuse anybody unlike present situation.

It could be documented that lib can be added to eager_load instead which works fine in production env.

gylaz added a commit to houndci/hound that referenced this issue Dec 23, 2016

Move several files from lib to app
Rails 5 does not autoload directories from `autoload_paths` when
`eager_load` is set (it is in production env).

One solution is to add `lib/` to `eager_load` paths, but that is deemed
non thread-safe (and also eager loads all the other code in `lib/` like
tasks, etc).

The other solution, and the one I went with, is to move the necessary
code to `app/`. Thus, I moved the there files to `app/models/`. You
could think of them as models, or other domain logic of our app, but
that's where I put them for now.

Read more about this Rails 5 issue:

* rails/rails#13142
* http://collectiveidea.com/blog/archives/2016/07/22/solutions-to-potential-upgrade-problems-in-rails-5/
@ngollan

This comment has been minimized.

Show comment
Hide comment
@ngollan

ngollan Jan 26, 2017

As @akostadinov mentioned above, the autoloader should be removed from Rails. It has never worked as advertised, and I do not see any chance of that thing ever working. In the past, it has demonstrated a complete misunderstanding of namespaces, injecting constants in wrong scopes and making it impossible to structure Rails projects without resorting to terminology from deep down the thesaurus.

At the very least, the "documentation" should be amended to have a big and clear warning that relying on anything but top-level constants inside app/ being picked up will lead to problems.

ngollan commented Jan 26, 2017

As @akostadinov mentioned above, the autoloader should be removed from Rails. It has never worked as advertised, and I do not see any chance of that thing ever working. In the past, it has demonstrated a complete misunderstanding of namespaces, injecting constants in wrong scopes and making it impossible to structure Rails projects without resorting to terminology from deep down the thesaurus.

At the very least, the "documentation" should be amended to have a big and clear warning that relying on anything but top-level constants inside app/ being picked up will lead to problems.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Jan 26, 2017

Member

@ngollan actually one of the long-standing issues (which is a Ruby problem and not a Rails problem) where a top level constant is found because of Object being in the ancestor chain of Class and not Module will be fixed in Ruby 2.5.

In my experience with working on a range of apps developed by other teams, any problems with autoloading has been down to incorrect additions to autoload_paths - just use folders inside app for stuff that's eager/auto loaded and lib for things that are manually required. That and strict mapping of namespace hierarchy to path names eliminates 99.99% of problems.

Autoloading in Rails is both the basis for eager loading in production and for reloading in development - I don't see the value in removing those features. If you want to manually control loading in your Rails app then it's perfectly possible to set up if you so desire, but it's not the default experience we want.

Member

pixeltrix commented Jan 26, 2017

@ngollan actually one of the long-standing issues (which is a Ruby problem and not a Rails problem) where a top level constant is found because of Object being in the ancestor chain of Class and not Module will be fixed in Ruby 2.5.

In my experience with working on a range of apps developed by other teams, any problems with autoloading has been down to incorrect additions to autoload_paths - just use folders inside app for stuff that's eager/auto loaded and lib for things that are manually required. That and strict mapping of namespace hierarchy to path names eliminates 99.99% of problems.

Autoloading in Rails is both the basis for eager loading in production and for reloading in development - I don't see the value in removing those features. If you want to manually control loading in your Rails app then it's perfectly possible to set up if you so desire, but it's not the default experience we want.

@akostadinov

This comment has been minimized.

Show comment
Hide comment
@akostadinov

akostadinov Jan 26, 2017

@pixeltrix , I was not specific in my last comment. For me it was frustrating that autoloading worked perfectly well in dev env but not in production env. In production some classes in a deeper namespace were missing. I think the way I setup auto-load [1] must be correct because it worked in dev env. As soon as I run server (even locally) in production mode, it was giving troubles. It might have something to do with the issue you link to, because I have one class with same name in top namespace and under a module. Or maybe just in production puma is using multiple workers.

In any case, if autoloading is not recommended as it appears from this thread, then do not advertise it in documentation. As soon as I've put lib in the eager load list instead of the auto-load list, things started to work properly in production env.

There is no official documentation listing the situations where autoloading might be an issue. I don't see how it makes sense to keep current situation - documentation advertises auto-loading, github thread discourages using it, there appears to be no incentive to fix auto-loading.

The easiest solution is to at least update documentation to recommend eager loading instead of auto loading. I can't see any reason for not not doing this.

[1] Here's how I actually do inside config/application.rb:

    # config.autoload_paths << "#{Rails.root}/lib"
    config.eager_load_paths << "#{Rails.root}/lib"

akostadinov commented Jan 26, 2017

@pixeltrix , I was not specific in my last comment. For me it was frustrating that autoloading worked perfectly well in dev env but not in production env. In production some classes in a deeper namespace were missing. I think the way I setup auto-load [1] must be correct because it worked in dev env. As soon as I run server (even locally) in production mode, it was giving troubles. It might have something to do with the issue you link to, because I have one class with same name in top namespace and under a module. Or maybe just in production puma is using multiple workers.

In any case, if autoloading is not recommended as it appears from this thread, then do not advertise it in documentation. As soon as I've put lib in the eager load list instead of the auto-load list, things started to work properly in production env.

There is no official documentation listing the situations where autoloading might be an issue. I don't see how it makes sense to keep current situation - documentation advertises auto-loading, github thread discourages using it, there appears to be no incentive to fix auto-loading.

The easiest solution is to at least update documentation to recommend eager loading instead of auto loading. I can't see any reason for not not doing this.

[1] Here's how I actually do inside config/application.rb:

    # config.autoload_paths << "#{Rails.root}/lib"
    config.eager_load_paths << "#{Rails.root}/lib"
@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Jan 26, 2017

Member

@akostadinov the recommendation is to create app/lib and drag all those files that need to work with reloading/autoloading/eagerloading into there from lib and not touch the config at all. We've taken out the comment about autoload_paths from the generated application.rb so the only way someone will find it is if they go looking for it.

Willing to accept a PR that places a great big warning in the relevant config section of the guide that basically says 'Here be dragons …' 😄

Member

pixeltrix commented Jan 26, 2017

@akostadinov the recommendation is to create app/lib and drag all those files that need to work with reloading/autoloading/eagerloading into there from lib and not touch the config at all. We've taken out the comment about autoload_paths from the generated application.rb so the only way someone will find it is if they go looking for it.

Willing to accept a PR that places a great big warning in the relevant config section of the guide that basically says 'Here be dragons …' 😄

@akostadinov

This comment has been minimized.

Show comment
Hide comment
@akostadinov

akostadinov Jan 27, 2017

@pixeltrix , I'm neither so knowledgeable about rails internal nor so good with english to update the guide. IMO a simple adding of a warning would not help a lot. In the doc link I posted originally, there is a whole section about autoload_paths and is mentioning auto-loading in various places. Very little is said about eager loading.

Also in this guide, there is no mention of app/lib nor google showed results when I was searching about app/lib.

Do you know where is a good place to file an issue about the aforementioned guide?

akostadinov commented Jan 27, 2017

@pixeltrix , I'm neither so knowledgeable about rails internal nor so good with english to update the guide. IMO a simple adding of a warning would not help a lot. In the doc link I posted originally, there is a whole section about autoload_paths and is mentioning auto-loading in various places. Very little is said about eager loading.

Also in this guide, there is no mention of app/lib nor google showed results when I was searching about app/lib.

Do you know where is a good place to file an issue about the aforementioned guide?

@ngollan

This comment has been minimized.

Show comment
Hide comment
@ngollan

ngollan Jan 31, 2017

@pixeltrix, a mechanism that will inject top-level constants into modules is defective, and not due to a "Ruby problem". Case in point, we tried having a class A::Process. I literally had the following happen in a console:

> A::Process == ::Process
false
> A::Process == ::Process
true

I am currently rewriting a medium-sized project to explicit loading of things in lib/ (which is funny because something will still finger around in there before explicit loading and provoke constant re-assignment warnings…), and stumbled upon another instance where lookup would be similarly broken.

This is not a Ruby problem! Plain Ruby does not arbitrarily screw up namespaces that badly; it takes a lot of work to create something as fragile and wrong.

ngollan commented Jan 31, 2017

@pixeltrix, a mechanism that will inject top-level constants into modules is defective, and not due to a "Ruby problem". Case in point, we tried having a class A::Process. I literally had the following happen in a console:

> A::Process == ::Process
false
> A::Process == ::Process
true

I am currently rewriting a medium-sized project to explicit loading of things in lib/ (which is funny because something will still finger around in there before explicit loading and provoke constant re-assignment warnings…), and stumbled upon another instance where lookup would be similarly broken.

This is not a Ruby problem! Plain Ruby does not arbitrarily screw up namespaces that badly; it takes a lot of work to create something as fragile and wrong.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Jan 31, 2017

Member

@ngollan that's not what I see with a clean Rails app:

$ rails new test_app
$ cd test_app
$ rails g model Post
$ rake db:migrate
$ rails c
Running via Spring preloader in process 81391
Loading development environment (Rails 5.0.1)
>> Post::Process == ::Process
(irb):1: warning: toplevel constant Process referenced by Post::Process
=> true
>> Post::Process == ::Process
(irb):2: warning: toplevel constant Process referenced by Post::Process
=> true
>> Post.constants.include?(:Process)
=> false

If you can reproduce your experience in a clean Rails app then please file a bug report.

Member

pixeltrix commented Jan 31, 2017

@ngollan that's not what I see with a clean Rails app:

$ rails new test_app
$ cd test_app
$ rails g model Post
$ rake db:migrate
$ rails c
Running via Spring preloader in process 81391
Loading development environment (Rails 5.0.1)
>> Post::Process == ::Process
(irb):1: warning: toplevel constant Process referenced by Post::Process
=> true
>> Post::Process == ::Process
(irb):2: warning: toplevel constant Process referenced by Post::Process
=> true
>> Post.constants.include?(:Process)
=> false

If you can reproduce your experience in a clean Rails app then please file a bug report.

@bsgreenb

This comment has been minimized.

Show comment
Hide comment
@bsgreenb

bsgreenb Apr 21, 2017

the recommendation is to create app/lib

This is Bad. The whole purpose of lib in the first place is to seperate non-domain-specific code from the app/ directory, which is domain specific.

Rails seems to be content to violate established best practices regarding the division between lib/ and app/. Messed up

bsgreenb commented Apr 21, 2017

the recommendation is to create app/lib

This is Bad. The whole purpose of lib in the first place is to seperate non-domain-specific code from the app/ directory, which is domain specific.

Rails seems to be content to violate established best practices regarding the division between lib/ and app/. Messed up

wtholt pushed a commit to bmic-development/sparc-request that referenced this issue May 19, 2017

William Holt
wth - (SPARCRequest) Code Improvement to Avoid Loading Lib for every
Page

With Rails 5, autoloading is now completely disabled if
config.eager_load = true. What this means is that if file paths outside
of app/ are in your config.autoload_paths and not in
config.eager_load_paths (for example, lib/), these files will no longer
load in deployed environments.

Moved ruby classes into app/lib per Rails team suggestions
(rails/rails#13142). Removed auto load
statements and in app settings set config.eager_load = true.
[#145461395]

wtholt pushed a commit to bmic-development/sparc-request that referenced this issue May 24, 2017

William Holt
wth - (SPARCRequest) Code Improvement to Avoid Loading Lib for every
Page

With Rails 5, autoloading is now completely disabled if
config.eager_load = true. What this means is that if file paths outside
of app/ are in your config.autoload_paths and not in
config.eager_load_paths (for example, lib/), these files will no longer
load in deployed environments.

Moved ruby classes into app/lib per Rails team suggestions
(rails/rails#13142). Removed auto load
statements and in app settings set config.eager_load = true.
[#145461395]
@krtschmr

This comment has been minimized.

Show comment
Hide comment
@krtschmr

krtschmr Jun 1, 2017

@bsgreenb so what would be your recommendation then? i do agree with you, that an external API wrapper should not be placed anywhere inside /app and personall i would rather see it in /lib

krtschmr commented Jun 1, 2017

@bsgreenb so what would be your recommendation then? i do agree with you, that an external API wrapper should not be placed anywhere inside /app and personall i would rather see it in /lib

@ashiksp

This comment has been minimized.

Show comment
Hide comment
@ashiksp

ashiksp Aug 2, 2017

Any link for best practise guidelines here?

ashiksp commented Aug 2, 2017

Any link for best practise guidelines here?

@sdhull

This comment has been minimized.

Show comment
Hide comment
@sdhull

sdhull Sep 19, 2017

Contributor

🙄 seems fine to move /lib inside /app. Just takes a minor adjustment in thinking, nbd.

Alternatively, you could do what my coworker did and add Dir[Rails.root.join('lib/**/*.rb')].each { |f| require f } to environment.rb
¯\_(ツ)_/¯

Contributor

sdhull commented Sep 19, 2017

🙄 seems fine to move /lib inside /app. Just takes a minor adjustment in thinking, nbd.

Alternatively, you could do what my coworker did and add Dir[Rails.root.join('lib/**/*.rb')].each { |f| require f } to environment.rb
¯\_(ツ)_/¯

no-reply added a commit to curationexperts/epigaea that referenced this issue Sep 21, 2017

Move autoloadable files from `lib` to `app/lib`
This is the recommended practice in Rails. See especially:
rails/rails#13142

no-reply added a commit to curationexperts/epigaea that referenced this issue Sep 21, 2017

Move autoloadable files from `lib` to `app/lib`
This is the recommended practice in Rails. See especially:
rails/rails#13142

TylerRick referenced this issue Mar 8, 2018

l0gicpath pushed a commit to l0gicpath/know_more that referenced this issue Jun 8, 2018

Hady Mahmoud
Move translator from lib dir to app/services
There's still a lot of hate/love confusion/lack-of-trust in the lib dir,
eager loading and autoloading thread safety since Rails 3.x

Without extensive testing in both production and development environments, I'm
not too comfortable leaving eager paths and autoload paths modified with the entire
lib directory.

Some more discussions around this: https://stackoverflow.com/q/38198668/1513477
And: rails/rails#13142
And: https://gist.github.com/maxivak/381f1e964923f1d469c8d39da8e2522f

For now, it's perfectly healthy to move translator service into a service
directory inside the app directory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment