Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

ActiveSupport::Dependencies.const_missing can choose incorrect from_mod #10685

Closed
trevorturk opened this Issue May 19, 2013 · 20 comments

Comments

Projects
None yet
7 participants
Contributor

trevorturk commented May 19, 2013

I noticed an issue while using the MailView gem, which someone discovered a few months ago: 37signals/mail_view#37

I traced the issue down to this commit: 2ed325a

The issue manifests as an exception "A copy of has been removed from the module tree but is still active!" ...which is coming from here: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/dependencies.rb#L444-L446

The MailView gem asks you to put a mailer in app/mailers like so:

class MessagePreview < MailView
  def example
    ExampleMailer.example
  end
end

If you change any model or controller in your application, you are forced to touch tmp/restart.txt to avoid raising the exception. I'm able to work around the issue by using the double-colon thingamajig to ensure we're at the top namespace:

class MessagePreview < MailView
  def example
    ::ExampleMailer.example
  end
end

...but notice it's done on the mailer. I did some debugging and noticed something strange in load_missing_constant like so:

load_missing_constant(from_mod, const_name)
from_mod = MessagePreview
const_name = ExampleMailer

So, I believe Dependencies is (incorrectly?) assuming that ExampleMailer is defined in MessagePreview when it is just a regular mailer defined like so:

class ExampleMailer < ApplicationMailer
  def example
  end
end

...which, I believe was changed by this commit: 2ed325a

I'm not sure if this is a bug with Rails or with MailView, to be honest. I haven't looked at the ActiveSupport::Dependencies code until now. Something seems fishy, though, so I thought I should open an issue here to get some more 👀 on it.

/cc @fxn

Owner

fxn commented May 19, 2013

@trevorturk ACK! At first sight it looks OK to me, because dependencies.rb emulates resolution in nesting, but I'll have a look as soon as I finish other stuff I am doing these days.

Contributor

trevorturk commented May 19, 2013

Awesome!

Please let me know if I can help.

I'll try to work up a test case since my steps to reproduce are pretty
convoluted right now :)

On May 19, 2013, at 12:25 PM, Xavier Noria notifications@github.com wrote:

@trevorturk https://github.com/trevorturk ACK! At first sight it looks OK
to me, because dependencies.rb emulates resolution in nesting, but I'll
have a look as soon as I finish other stuff I am doing these days.


Reply to this email directly or view it on
GitHubhttps://github.com/rails/rails/issues/10685#issuecomment-18121245
.

twmills commented May 26, 2013

I am seeing this too on Rails4-rc1/Ruby2 when using a custom route constraint that references and ActiveRecord class. Here's an example constraint:

class AdminConstraint
  def matches?(request)
    User.find(request.session['user_id']).admin?
  end
end

And the route definition:

constraints(AdminConstraint.new) do
  namespace :admin do
    resources :posts
  end
end

Referencing the class as ::User also fixed the issue for me.

Owner

fxn commented May 26, 2013

If you do not put ::User, which constant is resolved?

Contributor

trevorturk commented May 27, 2013

(This may be another issue for the 4.0.0 milestone.)

twmills commented May 27, 2013

@fxn I'm not sure how to determine that, but I did create an example app for you to recreate the issue easily:

https://github.com/twmills/rails-issue-10685

Hope that helps.

Owner

rafaelfranca commented May 27, 2013

Owner

fxn commented May 28, 2013

I am back to the project now, will have a look.

Owner

dhh commented Jun 3, 2013

@fxn, did you get a chance to look into this?

@fxn fxn was assigned Jun 3, 2013

Owner

fxn commented Jun 3, 2013

@dhh not yet, will prioritize it.

Owner

fxn commented Jun 3, 2013

Followup to say I have been able to reproduce using mail_view as described. I believe the double colon is a red-herring though, let me explain.

Active Support is able to load correctly the ExampleMailer constant, the code wouldn't work otherwise. You see MessagePreview mentioned in the trace because dependecies.rb kind of emulates looking up the nesting as Ruby does, so first it checks the most nested namespace, which is the one that got the const_missing hook called. That's MessagePreview. If there is no message_preview/example_mailer.rb in autoload_paths it goes up to the next outer namespace, ultimately checking Object. Since there is an example_mailer.rb in autoload_paths it gets interpreted and the mailer defined.

Without the double colon, the constant name is relative. With the double colon you are telling Ruby to look into Object directly, so MessagePreview.const_missing is not invoked. In particular, we are bypassing accessing the class MessagePreview altogether.

That's the key I believe, it suggests to me that MessagePreview is an autoloaded constant whose class object is being cached somewhere in Rails internals and becomes stale for some reason. That is, the class object itself is stored somewhere, its name is "MessagePreview", but dependencies.rb detects it is no longer reachable using the corresponding constant.

I notice both examples have in common that the constant whose class object becomes stale is used in config/routes.rb, maybe that is meaningful.

Will write back as soon as I do any advance.

Contributor

trevorturk commented Jun 3, 2013

Interesting! Thank you so much for working on this.

I wanted to point out this issue as well: 37signals/mail_view#6

Perhaps this is a mail_view issue or another red herring, but I was surprised that this issue needs extra work (expected Rails to automatically reload my mail_view files as it does my models etc.)

Sorry if that confuses things even further, but I thought I should mention it.

Owner

fxn commented Jun 4, 2013

I believe I understand what happens. As you said, this happens after commit 2ed325a, but if I am correct that commit actually uncovered an issue.

The key observation is that constant autoloading does not trigger routes reloading.

So the sequence is this: When the application boots in development mode MessagePreview is autoloaded because it is mentioned in config/routes.rb. When some application file is touched autoloaded constants are wiped, therefore MessagePreview is removed from Object. But since routes are not reloaded the class object that was stored in that constant is alive and still pointing to the configured path. Not reachable using the constant, but alive.

When a later request comes for action example the class that is serving that request is the cached class object whose corresponding constant is gone. The ExampleMailer constant is unknown, so const_missing is triggered on the orphan class object, and Active Support detects it is stale.

The solution to that is not to have stale objects, probably routing reloading should be also triggered. Will think about it and implement a solution.

Contributor

trevorturk commented Jun 4, 2013

Yes -- that makes perfect sense!

@fxn fxn closed this in b9b06da Jun 6, 2013

Contributor

trevorturk commented Jun 6, 2013

👍 🤘 🎉

Contributor

trevorturk commented Jun 7, 2013

It works! Thank you so much @fxn! That was a tricky one to track down, and I'm so happy we found and fixed the root issue. Super awesome!

@trevorturk trevorturk referenced this issue in basecamp/mail_view Jun 7, 2013

Closed

Doesn't update without server restart #6

Member

zzak commented Jun 7, 2013

Member

jonleighton commented Oct 21, 2013

Hey @fxn, I'm trying to upgrade my app to Rails 4 and seem to be bumping into a variant of this bug. I have a helper module called FormsHelper and when I reference another class from inside a method in FormsHelper, I get the error (when reloading has happened).

Do you have any tips about how to debug this? I know that something must be holding on to the previous FormsHelper, but I'm not sure how to track down that something.

Thanks

Member

jonleighton commented Oct 21, 2013

Don't worry, figured it out. I had a stray require that should have been a require_dependency.

Owner

fxn commented Oct 21, 2013

@jonleighton awesome, thanks for the followup.

@gsamokovarov gsamokovarov added a commit to gsamokovarov/rails that referenced this issue Mar 1, 2015

@gsamokovarov gsamokovarov Work around for upstream Ruby bug #10685
In f6e293e we avoided a segfault in the
tests, however I think we should try to avoid the crash, as it may
happen in user code as well.

Here is what I distiled the bug down to:

```ruby
# Rails case - works on 2.0, 2.1; crashes on 2.2
require 'action_dispatch'

ActionDispatch::Response.new(200, "Content-Type" => "text/xml")

# General case - works on 2.0, 2.1; crashes on 2.2
def foo(optional = {}, default_argument: nil)
end

foo('quux' => 'bar')
```

Pinging @jeremy and @eileencodes.
fafac27

@gsamokovarov gsamokovarov added a commit to gsamokovarov/rails that referenced this issue Mar 1, 2015

@gsamokovarov gsamokovarov Work around for upstream Ruby bug #10685
In f6e293e we avoided a segfault in the
tests, however I think we should try to avoid the crash, as it may
happen in user code as well.

Here is what I distiled the bug down to:

```ruby
# Rails case - works on 2.0, 2.1; crashes on 2.2
require 'action_dispatch'

ActionDispatch::Response.new(200, "Content-Type" => "text/xml")

# General case - works on 2.0, 2.1; crashes on 2.2
def foo(optional = {}, default_argument: nil)
end

foo('quux' => 'bar')
```

Pinging @jeremy and @eileencodes.
179beed

@gsamokovarov gsamokovarov added a commit to gsamokovarov/rails that referenced this issue Mar 1, 2015

@gsamokovarov gsamokovarov Work around for upstream Ruby bug #10685
In f6e293e we avoided a segfault in the
tests, however I think we should try to avoid the crash, as it may
happen in user code as well.

Here is what I distiled the bug down to:

```ruby
# Rails case - works on 2.0, 2.1; crashes on 2.2
require 'action_dispatch'

ActionDispatch::Response.new(200, "Content-Type" => "text/xml")

# General case - works on 2.0, 2.1; crashes on 2.2
def foo(optional = {}, default_argument: nil)
end

foo('quux' => 'bar')
```

Pinging @jeremy and @eileencodes.
9af54f8

@gsamokovarov gsamokovarov added a commit to gsamokovarov/rails that referenced this issue Mar 1, 2015

@gsamokovarov gsamokovarov Work around for upstream Ruby bug #10685
In f6e293e we avoided a segfault in the
tests, however I think we should try to avoid the crash, as it may
happen in user code as well.

Here is what I distiled the bug down to:

```ruby
# Rails case - works on 2.0, 2.1; crashes on 2.2
require 'action_dispatch'

ActionDispatch::Response.new(200, "Content-Type" => "text/xml")

# General case - works on 2.0, 2.1; crashes on 2.2
def foo(optional = {}, default_argument: nil)
end

foo('quux' => 'bar')
```
707a433

@eileencodes eileencodes added a commit that referenced this issue Mar 2, 2015

@eileencodes eileencodes Merge pull request #19147 from gsamokovarov/work-around-ruby-10695
Work around for upstream Ruby bug #10685
f32b6e2

@gsamokovarov gsamokovarov added a commit to gsamokovarov/rails that referenced this issue Mar 5, 2015

@gsamokovarov gsamokovarov Revert work arounds for upstream Ruby 2.2.0 kwargs bug
The bug caused a segfault and you can find more info about it at:
https://bugs.ruby-lang.org/issues/10685.

We did a couple of work arounds, but 2.2.1 rolled out and those aren't
needed anymore.

Here are the reverted commits:

- Revert "Work around for upstream Ruby bug #10685",
  commit 707a433.

- Revert "Fix segmentation fault in ActionPack tests",
  commit 22e0a22.
c01a050

@gsamokovarov gsamokovarov added a commit to gsamokovarov/rails that referenced this issue Mar 5, 2015

@gsamokovarov gsamokovarov Revert work arounds for upstream Ruby 2.2.0 kwargs bug
The bug caused a segfault and you can find more info about it at:
https://bugs.ruby-lang.org/issues/10685.

We did a couple of work arounds, but 2.2.1 rolled out and those aren't
needed anymore.

Here are the reverted commits:

- Revert "Work around for upstream Ruby bug #10685",
  commit 707a433.

- Revert "Fix segmentation fault in ActionPack tests",
  commit 22e0a22.
544f56e

@gsamokovarov gsamokovarov added a commit to gsamokovarov/rails that referenced this issue Mar 5, 2015

@gsamokovarov gsamokovarov Revert work arounds for upstream Ruby 2.2.0 kwargs bug
The bug caused a segfault and you can find more info about it at:
https://bugs.ruby-lang.org/issues/10685.

We did a couple of work arounds, but 2.2.1 rolled out and those aren't
needed anymore.

Here are the reverted commits:

- Revert "Work around for upstream Ruby bug #10685",
  commit 707a433.

- Revert "Fix segmentation fault in ActionPack tests",
  commit 22e0a22.

I'm also bumping the Ruby version check to 2.2.1 to prevent future
segfaults.
fae84cc

@gsamokovarov gsamokovarov added a commit to gsamokovarov/rails that referenced this issue Mar 5, 2015

@gsamokovarov gsamokovarov Revert work arounds for upstream Ruby 2.2.0 kwargs bug
The bug caused a segfault and you can find more info about it at:
https://bugs.ruby-lang.org/issues/10685.

We did a couple of work arounds, but 2.2.1 rolled out and those aren't
needed anymore.

Here are the reverted commits:

- Revert "Work around for upstream Ruby bug #10685",
  commit 707a433.

- Revert "Fix segmentation fault in ActionPack tests",
  commit 22e0a22.

I'm also bumping the Ruby version check to 2.2.1 to prevent future
segfaults.
8ed0b89

@brainopia brainopia added a commit to brainopia/rails that referenced this issue Mar 25, 2015

@gsamokovarov @brainopia gsamokovarov + brainopia Work around for upstream Ruby bug #10685
In f6e293e we avoided a segfault in the
tests, however I think we should try to avoid the crash, as it may
happen in user code as well.

Here is what I distiled the bug down to:

```ruby
# Rails case - works on 2.0, 2.1; crashes on 2.2
require 'action_dispatch'

ActionDispatch::Response.new(200, "Content-Type" => "text/xml")

# General case - works on 2.0, 2.1; crashes on 2.2
def foo(optional = {}, default_argument: nil)
end

foo('quux' => 'bar')
```
5531c75
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment