Refactored DeviceCache to improve thread safety#983
Conversation
taquitos
left a comment
There was a problem hiding this comment.
I think this is great. Before we merge, I'd love to see a new markdown doc in docs/ that describes the relationships between Atomic, Lock, and UserDefaults. I'm thinking something like an ELI5- keeping in mind that as we onboard junior engineers, they likely won't have a lot of exposure to generics/atomicity.
|
Thanks for the review!
Totally 👍 will do
Great idea! Will do.
|
|
ELI5: Literally: Explain like I'm five years old: https://www.reddit.com/r/explainlikeimfive/ |
|
that explains a lot 😂 I googled ELI5 and this python library popped up, couldn't figure out how that would apply here |
2a1bc08 to
54adb78
Compare
Done! Let me know what you think: https://github.com/RevenueCat/purchases-ios/blob/3d86e2635398623350a314e71205db8761eee662/docs/ThreadSafety.md |
Done. I've only included very simple tests that don't cover thread-safety itself. Such tests can be brittle and, if the implementation or the test is incorrect, lead to sporadic failures. But let me know if you think we should still have something to cover that. |
aboedo
left a comment
There was a problem hiding this comment.
looks awesome! Thanks for the updates!
e4f77b0 to
c18d77e
Compare
|
Thanks for the suggestions! I like adding the expected results in the test name 👍 |
See RevenueCat#983. This is less error-prone as it's impossible to have deadlocks, or to access the data without locking (like we were doing in some cases). Also, this approach has 2 separate locks for each property, so unrelated accesses won't lock each other now.
See #983. This is less error-prone as it's impossible to have deadlocks, or to access the data without locking (like we were doing in some cases). Also, this approach has 2 separate locks for each property, so unrelated accesses won't lock each other now.
See #983. This is less error-prone as it's impossible to have deadlocks, or to access the data without locking (like we were doing in some cases). Also, this approach has 2 separate locks for each property, so unrelated accesses won't lock each other now.
Fixes #739.
Changes:
Atomic, based onLock, to abstract atomic access and modifications to values.InMemoryCachedObjectusingAtomic(tests still pass!).SynchronizedUserDefaultsusingAtomic, to enforce atomic access toUserDefaults.DeviceCacheusingAtomicto enforce all access toUserDefaultsare synchronized:DeviceCacheno longer stores aUserDefaultsinstance (onlySynchronizedUserDefaults), so we guarantee we don't do unsynchronized modifications.threadUnsafe_methods are nowstatic, which ensures they don't access any local state. Instead,UserDefaultsis injected, andDeviceCachecalls these methods usingSynchronizedUserDefaults.read/SynchronizedUserDefaults.write.DeviceCachereads data, and THEN writes it again, without synchronizing both operations. This PR doesn't address that, but I haven't found any examples of that being an issue, and if we do, we should move both operations into a single method inDeviceCachethat can enforce the semantics.