-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Correctly read the cache_format_version
setting on boot
#45293
Conversation
Urgh, thanks. We really need to dig into the root cause for this, it's not normal that most settings are broken now. Something must have changed around Rails 7. |
Ah my bad. |
Hmm, should we consider changing the If someone uncommented this line: Line 42 in d15a694
They would still be using the So given that, I think we should push the new coder being the default down to Rails 7.1, and keep the old coder as the Rails 7 default along with this fix. I can do a PR for this if you agree. |
But then we'd break people who have |
No we wouldn't, because currently if you do |
WAT? From |
Yeah, that's what this PR fixes - there's no initializers involved here at all. edit: there is now - I pushed another test. Even with this fix, updating the new framework defaults will still give you the wrong coder. |
14c113c
to
f8984be
Compare
So I just tried a new Rails 7 app, and AFAICT it works:
|
🤦 sorry, I didn't finish my coffee, that's not a proper way to check, because it's the value when the cache store is instantiated that matters. |
Ok, let's go with this. Do you wanna handle it? |
There's still one problem though, since cache is initialized so early, there's no way to configure it it from |
Can do.
Yeah I’m also struggling to think of a solution for this. My best idea so far is a REALLY BIG WARNING in the new framework defaults file :/ |
The thing is, it happens so early that even I think we'll either have to delay it, or to initialize it twice (but that later may cause problem as people might write incompatible payloads in initializers). But to be honest the justification to have it so early seem weird. I really don't see an use case for accessing the cache as part of the boot. |
I am still waking up too but I think load_defaults does (with this patch) work. But initializers definitely don’t. Your point about cache being set up so early is fair though. That would resolve the issue totally, if we could delay it. I assume it’s done like that so you can do Rails.cache…. in initializers. I dunno why you’d want to but I suppose it’s there 🤷♂️ |
Yeah, but that's a bad idea. You shouldn't do any network access during boot. |
f8984be
to
7b5f9f7
Compare
Alright, I implemented this. The I also made #45304 which I think might decrease the impact of the current broken state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor thing, but LGTM.
This whole thing is really unfortunate, but I don't see any better way forward.
Quick brain dump on this. I tried moving the cache initializer from |
@ghiculescu actually I think we can simply fix the But someone upgrading from Rails 7 to Rails 7.1 won't have this problem. Same if we backport this fix, it won't be a problem. |
So it is. I missed that the first 10000000 times I thought about this. |
7b5f9f7
to
02e1366
Compare
02e1366
to
08352a2
Compare
@byroot I changed it back. What do you think about #45304 ? Given it definitely doesn't work in an initializer I still lean toward removing it from the file to avoid confusion. |
Backported as cb1a753. |
As noted in rails#45293 this setting doesn't work in initializers, nor in `config/application.rb`, in Rails 7. Anyone who has set `Rails.application.config.active_support.cache_format_version = 7.0` is still using the Rails 6.1 coder. This PR removes it from new framework defaults, so that users don't set it and expect it to do something. In a future PR I will be making this the new default in 7.1 as discussed [here](rails#45293 (comment)).
Thanks for bearing with me on this one, it was starting to hurt my brain. |
Same 😅. |
Summary
Fixes #45289
Other Information
I'm not sure if it's an antipattern to read from the config of specific modules in
railties/lib/rails/application/bootstrap.rb
. But we know for sure Active Support has loaded by then (I think), and this seems like the only way to ensure that this config is used correctly.