Break cycles when autosaving #7824

Closed
wants to merge 1 commit into
from

Projects

None yet

8 participants

@al2o3cr
Contributor
al2o3cr commented Oct 2, 2012

This is a draft of a fix for #7809 - there's still one failing test, but I'm not certain how to fix it.

The failing test verifies that only a single query is executed when updating a record with autosave on its assocations. Adding :inverse_of causes the related objects (Birds, in this case) to pick up the still-saving parent object (Pirate in this case) and save it again.

The net result is that duplicate UPDATE queries are sent. While this is certainly an improvement over the existing behavior (which recurses infinitely in changed_for_autosave?) it's still wrong.

Thoughts on how to fix this? The parent record clearly should be savable again, to accommodate code that updates the parent in, say, an after_save on child records. define_non_cyclic_method already avoids the cycle in that case, so the saves don't recurse indefinitely.

@frodsan frodsan commented on the diff Oct 26, 2012
activerecord/test/models/pirate.rb
@@ -21,7 +21,7 @@ class Pirate < ActiveRecord::Base
has_one :ship
has_one :update_only_ship, :class_name => 'Ship'
has_one :non_validated_ship, :class_name => 'Ship'
- has_many :birds, -> { order('birds.id ASC') }
+ has_many :birds, -> { order('birds.id ASC') }, :inverse_of => :pirate
@frodsan
frodsan Oct 26, 2012 Contributor

Please, use 1.9 hash syntax here. Thanks.

@jonleighton jonleighton was assigned Nov 30, 2012
@rafaelfranca
Member

@jonleighton mind to review this pull request?

@al2o3cr
Contributor
al2o3cr commented Nov 30, 2012

@rafaelfranca This is not entirely mergeable in its present state; there's a broken test due to the repeated save issue mentioned in my original note.

My thought is to move the :inverse_of setup from it's current home in Bird / Pirate to a separate / variant set of models to avoid breaking tests that aren't directly relevant to this issue. Will have some cycles to deal with this this weekend.

@schneems
Member

@al2o3cr do you have any updates to this issue/code?

@al2o3cr
Contributor
al2o3cr commented Feb 5, 2013

@schneems - been busy, but trying to get it to reproduce in isolation as adding inverse_of to Pirate irritated unrelated tests. Should have an update before this weekend.

@JonRowe
Contributor
JonRowe commented Mar 16, 2013

@al2o3cr did you manage to reproduce this in isolation?

@ghost
ghost commented Aug 28, 2013

thanks for solution, seems to work

@schuetzm schuetzm added a commit to schuetzm/acts_as_relation that referenced this pull request Nov 2, 2013
@schuetzm schuetzm Automatically add `:inverse_of`
This needs the following bug in Rails fixed first:
rails/rails#7809

Fix is in PR:
rails/rails#7824
8a7c442
@garysweaver
Contributor

@al2o3cr any update on this? Thanks!

@al2o3cr
Contributor
al2o3cr commented Jan 9, 2014

@garysweaver - I need to check this against Rails 4 as a bunch of code has been rewritten. Will update soon.

@rafaelfranca rafaelfranca added the stale label May 20, 2014
@rails-bot

This issue has been automatically closed because of inactivity.

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.

@rails-bot rails-bot closed this May 27, 2014
@garysweaver
Contributor

@rails-bot, thank you for closing older issues without assessing them, under the assumption that a bug only exists if someone has time to validate that it still exists. I especially like that we were all thanked for our contributions. Nice touch. But, I guess I am talking to myself?

@rafaelfranca
Member

The issue still exists, we know but we can't leave open until someone want to fix it. By the last comment from Jan 9, this PR is not even applicable anymore so I thought it is better to close. Also we gave a warning about this closing May 1 and until the time I closed they issues no comments.

I'm sorry if this is not sufficient to you but we are trying our best to help everyone.

@garysweaver
Contributor

@rafaelfranca I understand, but I think by doing this, the core team will lose sight of bugs that exist.

In-general outside of this specific ticket, there are two problems that need to be solved:

  1. The core team and others need to focus on issues where there is someone actively concerned about the issue and able to test it. In this regard, it helps to remove issues where there is no one active on the issue, so @rails-bot closing issues like this is a great thing.
  2. There are issues that are still valid and should be tracked and eventually addressed, however there is no one actively concerned about the issue; perhaps a fix was PR'd but no one on the core team could look into it at the time. In this case, the issues should not be closed and put into the same buckets as issues that are no longer issues because they have been fixed or those that are invalid.

Number 2 is what I'm talking about, and it is not just something insufficient to me personally- this is something that should be insufficient for every user of Rails and the core team, because it may be hiding valid bugs. Is there anything about this issue that flags this issue as "might be a valid issue even though it is closed, and maybe we should look at it later"? And, if not, should there be?

@rafaelfranca
Member

You just described what we are doing. This PR is invalid and the issue may be already fixed on master since a bunch of code has been rewritten. This is why this PR and the issue were closed. The other cases we did exactly what you described.

If it is really an issue, who care about it should try to reproduce at the master branches and report back so we could make sure it is still valid. If you are really concerned as you already showed please make sure you have a reproduction script and report at #7809 so we can reopen it.

@garysweaver
Contributor

@rafaelfranca So you are saying that the core team has decided to close valid bug reports if there is a lack of activity, and to reopen them requires a script or test provided to reproduce the issue in a supported version of Rails. That is ok, and I understand.

I had misunderstood in thinking that there was value in these bug reports and PRs, and I guess there isn't, because nothing is planned to be done about them.

@rafaelfranca
Member

We only need to make sure it is still reproduced in supported versions. I could do this for every stale issue of the repository, but that would be an insane amount of work. This is why we are asking help from the people who care about the bug more than us (because they are having the problems). Any way to reproduce is fine (script, applications test, or running you code against the supported branches). A simple "I still can reproduce this" comment would be sufficient to us remove the stale tag.

@garysweaver
Contributor

@rafaelfranca Ok, thanks!

@rafaelfranca
Member

BTW, lets focus in the real problem. Do you still have this problem with Rails 4+? If so I'll reopen the original issue.

@garysweaver
Contributor

Other than what others recorded in this issue, I don't have records of the original problem to be able to reproduce. I thought we were using something like update_attribute, save, etc. in a callback that in turn was calling callbacks and would result in SystemStackError: stack level too deep because callbacks happen whether it is dirty or not. I don't think that was necessarily the case, and I'm not sure exactly what was triggering the autosave, but here's an example test using ActiveRecord 4.1.1 that shows a scenario when code in a callback calls save that calls callbacks and gets into a loop just because inverse_of is doing autosaves. I don't think that is a bug, since ActiveRecord validly calls callbacks, and that can be avoided if you check changed? before save in the callback or if you use update_column(s) or execute SQL.

So, I guess it is ok to stay closed. I've not checked the original test that was contributed that was showing the problem, and I'm not sure about the others that have had this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment