Proposed fix for #5261 (Identity Map) #5442

Closed
rywall opened this Issue Mar 15, 2012 · 14 comments

Comments

Projects
None yet
8 participants
@rywall
Contributor

rywall commented Mar 15, 2012

Hi,

I've written a potential fix for the issue that caused the identity map to be removed: https://gist.github.com/2039089

It's currently written as an after_save callback on the comment model but I would be willing to add it into activerecord with tests, etc, if the general idea of it is acceptable to the core team.

The general idea is that after a record is saved (if IdentityMap is enabled), its reflections are checked for any belongs_to associations that have a foreign_key (or foreign_type) that was changed. These associated models have their entire association cache cleared. This is probably heavy handed and could be optimized to only clear associations that rely on the changed foreign_key.

This fixes the issue mentioned in #5261 and also makes Rails work more logically than when the Identity Map is disabled in that situation.

Specifically:

Comment.create(:commentable => Post.create!)

ActiveRecord::IdentityMap.use do
  post = Post.first
  comment = post.comments[0]
  comment.commentable = nil
  comment.save
  post.comments # returns [] with IM on and [Comment] with IM off
end

Please let me know if the issue is more complicated than I've understood it to be.

@njakobsen

This comment has been minimized.

Show comment Hide comment
@njakobsen

njakobsen Mar 15, 2012

Contributor

+1

Contributor

njakobsen commented Mar 15, 2012

+1

@rywall

This comment has been minimized.

Show comment Hide comment
@rywall

rywall Mar 18, 2012

Contributor

@josevalim this should fix the issue you documented in 302c912.

Contributor

rywall commented Mar 18, 2012

@josevalim this should fix the issue you documented in 302c912.

@arturopie

This comment has been minimized.

Show comment Hide comment
@arturopie

arturopie Mar 19, 2012

Contributor

Watching

Contributor

arturopie commented Mar 19, 2012

Watching

@sskirby

This comment has been minimized.

Show comment Hide comment
@sskirby

sskirby Mar 19, 2012

Contributor

+1

Contributor

sskirby commented Mar 19, 2012

+1

@njakobsen

This comment has been minimized.

Show comment Hide comment
@njakobsen

njakobsen Mar 19, 2012

Contributor

I think the next question is, where would be the best place to do this? An after save is probably a little too loosely integrated, or is it? Does anyone see any potential pitfalls with just clearing the cached associations of the related models?

Contributor

njakobsen commented Mar 19, 2012

I think the next question is, where would be the best place to do this? An after save is probably a little too loosely integrated, or is it? Does anyone see any potential pitfalls with just clearing the cached associations of the related models?

@tommeier

This comment has been minimized.

Show comment Hide comment
@tommeier

tommeier Mar 20, 2012

Contributor

Awesome!

Contributor

tommeier commented Mar 20, 2012

Awesome!

@ghost ghost assigned guilleiguaran Apr 29, 2012

@guilleiguaran

This comment has been minimized.

Show comment Hide comment
@guilleiguaran

guilleiguaran May 4, 2012

Owner

I will create a branch based in master and will apply all the patches related to Identity Map, if we can fix the pending bugs in the next months we can ask for re-add it to Rails master.

Owner

guilleiguaran commented May 4, 2012

I will create a branch based in master and will apply all the patches related to Identity Map, if we can fix the pending bugs in the next months we can ask for re-add it to Rails master.

@sskirby

This comment has been minimized.

Show comment Hide comment
@sskirby

sskirby May 4, 2012

Contributor

My employer has graciously offered to fund the fixing of the IdentityMap by allowing me to work on it after my current project is over. So if everything isn't already fixed in 2 weeks time I will have time to work on it.

Contributor

sskirby commented May 4, 2012

My employer has graciously offered to fund the fixing of the IdentityMap by allowing me to work on it after my current project is over. So if everything isn't already fixed in 2 weeks time I will have time to work on it.

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva May 16, 2012

Owner

@sskirby great, let us know if you need help with anything :)

@sskirby great, let us know if you need help with anything :)

@rapind

This comment has been minimized.

Show comment Hide comment
@rapind

rapind May 22, 2012

Go Kirby!

rapind commented May 22, 2012

Go Kirby!

@njakobsen

This comment has been minimized.

Show comment Hide comment
@njakobsen

njakobsen May 22, 2012

Contributor

From what I've seen of the identity map, it looks like if we want a surefire way to keep everything consistent with regards to attributes & associations, we'd need to retain a single handle to the activerecord object once it's instantiated. That means, if we ever clear the map, we'd actually need to keep the records, just mark them as dirty, and then record.reload them when they're needed. That way multiple instances of the same record don't exist in memory, which is where some of these problems are originating from.

I know @wycats mentioned at Railsconf 2012 he was solving a lot of the identity map issues in his new project. Does anyone have a line on him and could get some knowledge transfer going?

Contributor

njakobsen commented May 22, 2012

From what I've seen of the identity map, it looks like if we want a surefire way to keep everything consistent with regards to attributes & associations, we'd need to retain a single handle to the activerecord object once it's instantiated. That means, if we ever clear the map, we'd actually need to keep the records, just mark them as dirty, and then record.reload them when they're needed. That way multiple instances of the same record don't exist in memory, which is where some of these problems are originating from.

I know @wycats mentioned at Railsconf 2012 he was solving a lot of the identity map issues in his new project. Does anyone have a line on him and could get some knowledge transfer going?

@sskirby

This comment has been minimized.

Show comment Hide comment
@sskirby

sskirby May 25, 2012

Contributor

I've been doing some investigations into keeping bi-directional associations in sync and I'm not sure this is the way to go. You either incur a lot of reloads when a foreign key changes or you have to write some complex piece of code that knows to remove and add child from the list of children maintained by it's parent (because you have to consider what happens when you move a child from one parent to another.)

Hibernate has a great identity map in the form of the Session cache and everything I've read indicates that they leave it up to the developer to keep bi-directional associations in sync. This seems like a reasonable caveat that would keep the Identity Map code as simple as possible.

My current though is to find all of the methods in AR that behave 'strangely' when the Identity Map is enabled and 'fix' them. In particular Post.destroy(@post.id) could be fixed by by-passing the Identity Map when loading the new Post to be destroyed.

Thoughts?

Contributor

sskirby commented May 25, 2012

I've been doing some investigations into keeping bi-directional associations in sync and I'm not sure this is the way to go. You either incur a lot of reloads when a foreign key changes or you have to write some complex piece of code that knows to remove and add child from the list of children maintained by it's parent (because you have to consider what happens when you move a child from one parent to another.)

Hibernate has a great identity map in the form of the Session cache and everything I've read indicates that they leave it up to the developer to keep bi-directional associations in sync. This seems like a reasonable caveat that would keep the Identity Map code as simple as possible.

My current though is to find all of the methods in AR that behave 'strangely' when the Identity Map is enabled and 'fix' them. In particular Post.destroy(@post.id) could be fixed by by-passing the Identity Map when loading the new Post to be destroyed.

Thoughts?

@sskirby

This comment has been minimized.

Show comment Hide comment
@sskirby

sskirby May 28, 2012

Contributor

@guilleiguaran do you know why we are removing records that don't save successfully from the IdentityMap?

Contributor

sskirby commented May 28, 2012

@guilleiguaran do you know why we are removing records that don't save successfully from the IdentityMap?

@guilleiguaran

This comment has been minimized.

Show comment Hide comment
@guilleiguaran

guilleiguaran Jun 28, 2012

Owner

I will close this here, lets continue the discussion in the PR

Owner

guilleiguaran commented Jun 28, 2012

I will close this here, lets continue the discussion in the PR

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