Return a copy of the cache entry when local_cache exists: #36656
Conversation
| @data.fetch(key) { @data[key] = yield } | ||
| entry = @data.fetch(key) { @data[key] = yield } | ||
| dup_entry = entry.dup | ||
| dup_entry&.dup_value! |
There was a problem hiding this comment.
How could dup_entry ever be nil?
There was a problem hiding this comment.
in case you try to access a key that doesn't exist
There was a problem hiding this comment.
Nevermind, I'm stuck in the past, turns out nil.dup actually exist on modern rubies TIL.
There was a problem hiding this comment.
I think this would have to be a deep copy. .dup alone will still let you modify nested objects if present
There was a problem hiding this comment.
No because dup_value! does a Marshal.load
- When the local cache exists (during the request lifecycle), the entry returned from the LocalStore is passed as a reference which means mutable object can accidentaly get modified. This behaviour seems unnecessarily unsafe and is prone to issues like it happened in our application. This patch dup the `Entry` returned from the cache and dup it's internal value.
Edouard-chin
force-pushed
the
ec-local-cache-reference
branch
from
913f53d
to
f08bf72
Compare
| @@ -75,7 +75,10 @@ def delete_entry(key, options) | |||
| end | |||
|
|
|||
| def fetch_entry(key, options = nil) # :nodoc: | |||
| @data.fetch(key) { @data[key] = yield } | |||
| entry = @data.fetch(key) { @data[key] = yield } | |||
| dup_entry = entry.dup | |||
There was a problem hiding this comment.
Why you need to dup the entry itself?
There was a problem hiding this comment.
the @data hash keeps a reference to the entry, if I don't dup it, user can modify the underlying value which would affect the cache.
The local cache need to protect against mutations of both the initial received value, and the value returned. See: - rails#36656 - rails#37587 Because of this, the overhead of the local cache end up being quite significant. On a single read, the value will be deep duped twice. So unless the value is one of the type benefiting from a fast path, we'll have to do two `Marshal.load(Marshal.dump(value))` roundtrips, which for large values can be very expensive. By using a specialized `LocalEntry` type instead, we can store the `Marshal` payload rather than the original value. This way the overhead is reduced to a single `Marshal.dump` on writes and a single `Marshal.load` on reads.
Return a copy of the cache entry when local_cache exists:
When the local cache exists (during the request lifecycle), the
entry returned from the LocalStore is passed as a reference which
means mutable object can accidentaly get modified.
This behaviour seems unnecessarily unsafe and is prone to
issues like it happened in our application.
This patch dup the
Entryreturned from the cache and dup it'sinternal value.
cc/ @rafaelfranca @casperisfine