Skip to content
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

Update the cached value for the key when value set #86

Merged
merged 1 commit into from Mar 10, 2016

Conversation

justin808
Copy link

If setting a cache value is called within a TX and then the value is
accessed, then the old value is retrieved from the cache.

The fix is to clear the cached value for the var name immediately after
calling save!. Note, it is still appropriate to clear cache values upon
commit due to race conditions.

Review on Reviewable

@justin808
Copy link
Author

@huacnlee Can you let me know if this looks good? Thanks.

@huacnlee
Copy link
Owner

I don't know what your means, please give a example codes for this case.

@justin808
Copy link
Author

See added specs, @huacnlee. Thanks.

@justin808
Copy link
Author

@huacnlee Have you had a chance to look at my PR?

@mameier
Copy link

mameier commented Feb 23, 2016

An easier way to ensure consistency of cached settings (even during transactions, and in test) is to write the specific cache entry when an attribute is changed.
See my pull request #88

@justin808
Copy link
Author

@huacnlee Any word?

@mameier Your suggestion would require modifying the code with a new API.

@mameier
Copy link

mameier commented Feb 27, 2016

The API is unchanged. All existing tests pass.
Only the behavior is what I (and some others) expect:
as soon as you set a setting, the new value is available.

Or do you mean that I should write a failing test that is fixed by the code change?

If setting a cache value is called within a TX and then the value is
accessed, then the old value is retrieved from the cache.

The fix is to set the cached value for the var name immediately after
calling save!.
@justin808 justin808 force-pushed the justin-clear-cache-key-on-save branch from 3e812a2 to 5640654 Compare March 1, 2016 05:21
@justin808 justin808 changed the title Clear the cache for the key when set Update the cached value for the key when value set Mar 1, 2016
@justin808
Copy link
Author

@huacnlee I rebased on top of master, and I included the suggestion in #88 from @mameier (good call!)

Can we get this merged and new release of the gem?

@tovodeverett
Copy link

This resolves some of the issue in my test suite, but not all of them. In particular, it appears that calls to Setting.destroy do not clear the cache for the key in question.

@justin808
Copy link
Author

@huacnlee Any word on this PR?

huacnlee added a commit that referenced this pull request Mar 10, 2016
Update the cached value for the key when value set
@huacnlee huacnlee merged commit 338346a into huacnlee:master Mar 10, 2016
@justin808
Copy link
Author

@huacnlee Any ETA on when you might push a new gem version? I've got a large production system that needs this change. Thanks!

@huacnlee
Copy link
Owner

@justin808 version 0.5.4

@justin808
Copy link
Author

@huacnlee Big thanks. Sorry I missed that! Have you considered tagging your versions. I do that with my gem: https://github.com/shakacode/react_on_rails/.

See https://github.com/shakacode/react_on_rails/blob/master/docs%2Freleasing.md

I use the gem "gem-release".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants