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

AR : Circular `dependent: :destroy` cause 'stack level too deep' error #13609

Closed
bobbus opened this Issue Jan 6, 2014 · 33 comments

Comments

Projects
None yet
Contributor

bobbus commented Jan 6, 2014

I believe this bug wasn't present before Rails 4.

See a full runnable gist here

Owner

rafaelfranca commented Jan 6, 2014

It make sense it cause this. You setup is wrong. Remove the dependent: :destroy from the AddressUserLink and everything will work fine.

Owner

rafaelfranca commented Jan 6, 2014

There is a warning in the guides:

You should not specify this option on a belongs_to association that is connected with a has_many association on the other class. Doing so can lead to orphaned records in your database.

Contributor

bobbus commented Jan 6, 2014

Thanks for your answer @rafaelfranca .

Actually this part of the guide does not apply to my case : I'm not setting dependent: :destroy on a belongs_to that is connected with a has_many but it's connected to an has_one.

Also, I should not end up with orphaned records in my DB with this setup.

Ideally, I would think this should be fixed as there is no reason to have a stack level too deep error but just have the two models properly removed. If it's seems too difficult to fix this way, maybe we should add a warning on guide / documentation about circular destroy ?

@rafaelfranca , thoughts ?

Owner

rafaelfranca commented Jan 6, 2014

There is a reason for stack level too deep:

When you call address.destroy it will call address_user_link.destroy that will call address.destroy and so on.

Will it work if we use delete?

A warning about circular destroy would be great

@rafaelfranca rafaelfranca reopened this Jan 6, 2014

Contributor

bobbus commented Jan 6, 2014

I understand the reason which end up with a stack level too deep, what I mean is ideally this should be avoid and just end up by the deletion of the two records.

Especially because this was what happen before Rails 4.

In my app, I use to add a custom after_destroy callback to remove linked object without raising a stack level too deep error (this works because of the after callback, if I remeber correctly, dependent: :destroy on relationship add a before_destroy callback.

I will try with other setup to see if circular dependency always raise stack level too deep error and add my findings here.

Member

byroot commented Jan 10, 2014

I just has the exact same issue while upgrading a codebase to rails 4.

I worked around with this:

after_destroy :destroy_mobile_device # workaround for https://github.com/rails/rails/issues/13609

def destroy_mobile_device
  return unless mobile_device
  mobile_device.destroy unless mobile_device.destroyed?
end

And I confirm it was previously working fine until 4.0.

Contributor

diatmpravin commented Feb 2, 2014

I just replicate this issue for later release including 4.0.0 for has_many and has_one relationship. It's fine for earlier release.

See a full runnable gist for has_many and has_one

The workarounds posted suggest using the after_destroy callback. But that doesn't work, are you guys sure this is working for you? The problem is that .destroyed? returns false always inside an after_destroy callback. So you'll still end up with a circular dependency and eventual stack level too deep error.

I have not found an easy workaround for this :(.

I wrote a StackOverflow question trying to find a workaround for this issue:
http://stackoverflow.com/questions/23208579/is-there-a-rails-4-circular-dependent-destroy-workaround

Member

byroot commented Apr 22, 2014

@samueller I does work for us as of Rails 4.0. I don't know for 4.1

dwilkie commented Apr 29, 2014

I had the same problem as @samueller with rails 4.02 and got around it with the following fix:

# internet_service.rb

class InternetService < ActiveRecord::Base
  has_one :subscription
  after_destroy :destroy_subscription

  private

  def destroy_subscription
    subscription.destroy if subscription && !subscription.destroyed? && !subscription.will_destroy?
  end
end

# subscription.rb
class Subscription < ActiveRecord::Base
  belongs_to :internet_service, :dependent => :destroy
  before_destroy :will_destroy

  def will_destroy?
    @will_destroy
  end

  private

  def will_destroy
    @will_destroy = true
  end
end

@dwilkie I had tried something very similar to this with both Rails 4.0.x and 4.1.0. Was hoping you had some slight twist which would make it work. Unfortunately this doesn't work for me :(. Essentially will_destroy never gets called.

dwilkie commented Apr 30, 2014

This solution described above works for me, but I had the same problem as you before where destroyed? was always returning false.

@dwilkie strange how for some people destroyed? works, for you the before_destroy at the time you expect it and for me nothing works... Even more surprising that Ruby on Rails doesn't deal with circular destroys better.

@rails-bot rails-bot added the stale label Aug 19, 2014

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

BelongsTo association should be fixed to use after_destroy to avoid circular dependency delete.

@jasonkim - makes sense. Seems to me that Rails/ActiveRecord could easily detect and avoid circular dependency delete issues without anybody having to implement anything in any after_destroy callback methods. I can confirm I'm still having this issue in Rails 4.0 and 4.1.

@robin850 robin850 removed the stale label Aug 22, 2014

@zetter zetter added a commit to Futurelearn/rails that referenced this issue Sep 8, 2014

@zetter zetter Failing test for circular dependent association bug.
Related issue: rails#13609

The tests fail with a stack overflow error because the destroy
callback in the ‘belongs to’ and ‘has one’ associations keep calling
each other.

Since both cases depend on the behaviour of the ‘has one’ and
‘belongs to’ associations I have put them together in a new test file.
cedf64e
Contributor

zetter commented Sep 8, 2014

Hi everyone,

I've also been caught by this unexpected behaviour. In the codebase I'm working on, we have two records that we'd like to ensure are always created or destroyed together. We've worked around this issue by removing one of the dependent: :destroy options and making sure we always trigger the destroy from the other record.

With help from @sebjacobs, I have made a branch that has some failing tests and a partial fix. The stops the stack error but still calls #destroy multiple times in some cases (see commit message) which is hard to avoid because of the use of a 'before destroy' callback (which perhaps makes this related to #670).

Is anyone able to suggest a way of completely fixing the issue?

If we can't find a fix for the issue then I agree with @bobbus that it would be good to add a warning in the documentation that circular dependent options are not supported. I'd be happy to suggest wording for this.

This problem haunts me, too. It looks like it shouldn't be that hard, but I haven't found a good solution yet... (other than replacing dependent: :destroy)

Wouldn't it be possible to use the same mechanism as autosave associations? They are subject to the same circular dependency issue, but there the save and validation methods are wrapped by some loop-detection code.

At ActiveRecord::AutosaveAssociation::ClassMethods.define_non_cyclic_method:

unless @_already_called[name]
  begin
    @_already_called[name]=true
    result = instance_eval(&block)
  ensure
    @_already_called[name]=false
  end
end

If I understand correctly, @zetter's patch detects a circular call implicitly by checking persisted?. Here it is done more explicitly by recording a previous call in @_already_called.

Contributor

sebjacobs commented Sep 11, 2014

After seeing the solution proposed by @daniel-rikowski I have come up with a quick sketch which involves making changes to ActiveRecord::Callbacks#destroy[1].

def destroy
  return if @_destroy_callback_already_called
  @_destroy_callback_already_called = true
  run_callbacks(:destroy) { super }
end

This change makes @zetter's spec pass and should be able to deal with HasMany and HasManyThrough associations too.

[1] https://github.com/Futurelearn/rails/blob/master/activerecord/lib/active_record/callbacks.rb#L288

Contributor

sebjacobs commented Sep 12, 2014

After chatting to @zetter this morning I can confirm that by ensuring that we set @destroy_callback_already_called to false we can handle the case of multiple calls to destroy on a given a object.

def destroy
  begin
    return if @_destroy_callback_already_called
    @_destroy_callback_already_called = true
    run_callbacks(:destroy) { super }
  ensure
    @_destroy_callback_already_called = false
  end
end

You beat me to it :) I was about to suggest the same thing (the ensure part)

I'll give it a try this weekend.

@zetter zetter added a commit to Futurelearn/rails that referenced this issue Sep 12, 2014

@zetter @sebjacobs zetter + sebjacobs Failing test for circular dependent association bug.
Related issue: rails#13609

The tests fail with a stack overflow error because the destroy
callback in the ‘belongs to’ and ‘has one’ associations keep calling
each other.

Since both cases depend on the behaviour of the ‘has one’ and
‘belongs to’ associations I have put them together in a new test file.
fc3d67e

@sebjacobs sebjacobs added a commit to Futurelearn/rails that referenced this issue Sep 12, 2014

@sebjacobs sebjacobs Fix callback circular dependency issue.
This commit includes changes to the `ActiveRecord::Callbacks#destroy`
method so that it only executes the associated callbacks once.

Prior to this commit a circular `dependent: :destroy` would result in a
stacktrace.

However this commit introduces a guard which prevents subsequent calls
to `run_callbacks(:destroy)` on a single instance.

This solution comes out of a discussion between @zetter,
@daniel-rikowski and I[1].

[1] rails#13609
6621c93

@sebjacobs sebjacobs added a commit to Futurelearn/rails that referenced this issue Sep 12, 2014

@sebjacobs sebjacobs Fix callback circular dependency issue.
This commit includes changes to the `ActiveRecord::Callbacks#destroy`
method so that it only executes the associated callbacks once.

Prior to this commit a circular `dependent: :destroy` would result in a
stacktrace.
e.g.

```ruby
class Content < ActiveRecord::Base
  has_one :content_position, dependent: :destroy
end

class ContentPosition < ActiveRecord::Base
  belongs_to :content, dependent: :destroy
end
```

However this commit introduces a guard which prevents subsequent calls
to `run_callbacks(:destroy)` on a single instance.

This solution comes out of a discussion between @zetter,
@daniel-rikowski and I[1].

[1] rails#13609
5e53a01

@sebjacobs sebjacobs added a commit to Futurelearn/rails that referenced this issue Sep 12, 2014

@sebjacobs sebjacobs Ensure destroy callback is set to false.
This builds upon the solution outlined in the previous commit and
the discussion between @zetter, @daniel-rikowski and I[1].

Prior to this commit destroy callbacks would only be run the first time
you attempted to destroy an object, which does not cater for the case
when a call to destroy fails.

This commit addresses this issue, so that destroy callbacks will be
triggered on subsequent calls to `destroy`.

[1] rails#13609
e861c06
Contributor

sebjacobs commented Sep 12, 2014

@daniel-rikowski I've pushed a branch[1] with the additional tweaks, would love to hear how you get on.
[1] https://github.com/Futurelearn/rails/compare/seb-fix-circular-dependent-destroy

👍 I just tested this and it works fine. No more endless recursion, i.e. the pre-4.0 behaviour seems to be restored. It's always a good feeling to delete workaround code.

Nice job!

Contributor

sebjacobs commented Sep 14, 2014

Coolio, I will spend some time tomorrow afternoon drafting a pull request.

+1

I'm having a similar issue, patching ActiveRecord::Callbacks like suggested by @sebjacobs does seems to solve it... http://stackoverflow.com/questions/27107094/rails-avoid-recursive-destroy-callbacks. This would be thread safe right?

Contributor

toshimaru commented Dec 17, 2014

+1 I'm encountering the problem...

Member

byroot commented Dec 17, 2014

For 4.1 our workaround now look like this:

after_destroy :destroy_mobile_device # workaround for https://github.com/rails/rails/issues/13609

def destroy_mobile_device
  return unless mobile_device
  device = mobile_device
  self.mobile_device = nil
  device.destroy unless device.destroyed?
end

Hope it helps.

@sebjacobs sebjacobs added a commit to sebjacobs/rails that referenced this issue Jan 16, 2015

@sebjacobs sebjacobs Add support for bidirectional destroy dependencies
Prior to this commit if you defined a bidirectional relationship
between two models with destroy dependencies on both sides, a call to
`destroy` would result in an infinite callback loop.

Take the following relationship.

    class Content < ActiveRecord::Base
      has_one :content_position, dependent: :destroy
    end

    class ContentPosition < ActiveRecord::Base
      belongs_to :content, dependent: :destroy
    end

Calling `Content#destroy` or `ContentPosition#destroy` would result in
an infinite callback loop.

This commit changes the behaviour of `ActiveRecord::Callbacks#destroy`
so that it guards against subsequent callbacks.

Thanks to @zetter for demonstrating the issue with failing tests[1].

[1] rails#13609
218c675

@sebjacobs sebjacobs added a commit to sebjacobs/rails that referenced this issue Jan 16, 2015

@sebjacobs sebjacobs Add support for bidirectional destroy dependencies
Prior to this commit if you defined a bidirectional relationship
between two models with destroy dependencies on both sides, a call to
`destroy` would result in an infinite callback loop.

Take the following relationship.

    class Content < ActiveRecord::Base
      has_one :content_position, dependent: :destroy
    end

    class ContentPosition < ActiveRecord::Base
      belongs_to :content, dependent: :destroy
    end

Calling `Content#destroy` or `ContentPosition#destroy` would result in
an infinite callback loop.

This commit changes the behaviour of `ActiveRecord::Callbacks#destroy`
so that it guards against subsequent callbacks.

Thanks to @zetter for demonstrating the issue with failing tests[1].

[1] rails#13609
9ba7b0d

@sebjacobs sebjacobs added a commit to sebjacobs/rails that referenced this issue Jan 16, 2015

@sebjacobs sebjacobs Add support for bidirectional destroy dependencies
Prior to this commit if you defined a bidirectional relationship
between two models with destroy dependencies on both sides, a call to
`destroy` would result in an infinite callback loop.

Take the following relationship.

    class Content < ActiveRecord::Base
      has_one :content_position, dependent: :destroy
    end

    class ContentPosition < ActiveRecord::Base
      belongs_to :content, dependent: :destroy
    end

Calling `Content#destroy` or `ContentPosition#destroy` would result in
an infinite callback loop.

This commit changes the behaviour of `ActiveRecord::Callbacks#destroy`
so that it guards against subsequent callbacks.

Thanks to @zetter for demonstrating the issue with failing tests[1].

[1] rails#13609
72fb85f

@sebjacobs sebjacobs added a commit to sebjacobs/rails that referenced this issue Jan 16, 2015

@sebjacobs sebjacobs Add support for bidirectional destroy dependencies
Prior to this commit if you defined a bidirectional relationship
between two models with destroy dependencies on both sides, a call to
`destroy` would result in an infinite callback loop.

Take the following relationship.

    class Content < ActiveRecord::Base
      has_one :content_position, dependent: :destroy
    end

    class ContentPosition < ActiveRecord::Base
      belongs_to :content, dependent: :destroy
    end

Calling `Content#destroy` or `ContentPosition#destroy` would result in
an infinite callback loop.

This commit changes the behaviour of `ActiveRecord::Callbacks#destroy`
so that it guards against subsequent callbacks.

Thanks to @zetter for demonstrating the issue with failing tests[1].

[1] rails#13609
8ed0a36

@sebjacobs sebjacobs added a commit to sebjacobs/rails that referenced this issue Jan 16, 2015

@sebjacobs sebjacobs Add support for bidirectional destroy dependencies
Prior to this commit if you defined a bidirectional relationship
between two models with destroy dependencies on both sides, a call to
`destroy` would result in an infinite callback loop.

Take the following relationship.

    class Content < ActiveRecord::Base
      has_one :content_position, dependent: :destroy
    end

    class ContentPosition < ActiveRecord::Base
      belongs_to :content, dependent: :destroy
    end

Calling `Content#destroy` or `ContentPosition#destroy` would result in
an infinite callback loop.

This commit changes the behaviour of `ActiveRecord::Callbacks#destroy`
so that it guards against subsequent callbacks.

Thanks to @zetter for demonstrating the issue with failing tests[1].

[1] rails#13609
d5bf649

vizo commented May 3, 2015

👍 +1

Contributor

bughit commented Jun 4, 2015

  1. It's not just callbacks, it seems to me that the entire operation should be protected both from reentry (destroy called while a previous destroy is still on the stack) and repeated invocations. Both should raise by default, but perhaps allow a silent exit via an option.
  2. Why is dependent destruction done via before_destroy rather than after_destroy? If it was done in after_destroy you could check the destroyed state of the associations.
Contributor

ioquatix commented Sep 21, 2015

+1

Contributor

gkop commented Sep 28, 2015

+1

@arthurnn arthurnn self-assigned this Oct 18, 2015

Member

sgrif commented Oct 28, 2015

Fixed by #18548

@sgrif sgrif closed this Oct 28, 2015

@fernandomm fernandomm added a commit to fernandomm/rails that referenced this issue Jan 7, 2016

@sebjacobs @fernandomm sebjacobs + fernandomm Add support for bidirectional destroy dependencies
Prior to this commit if you defined a bidirectional relationship
between two models with destroy dependencies on both sides, a call to
`destroy` would result in an infinite callback loop.

Take the following relationship.

    class Content < ActiveRecord::Base
      has_one :content_position, dependent: :destroy
    end

    class ContentPosition < ActiveRecord::Base
      belongs_to :content, dependent: :destroy
    end

Calling `Content#destroy` or `ContentPosition#destroy` would result in
an infinite callback loop.

This commit changes the behaviour of `ActiveRecord::Callbacks#destroy`
so that it guards against subsequent callbacks.

Thanks to @zetter for demonstrating the issue with failing tests[1].

[1] rails#13609
c2873bd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment