Prevent deadlocks in SynchronizedUserDefaults #5858
Conversation
Generated by 🚫 Danger |
📸 Snapshot Test241 unchanged
🛸 Powered by Emerge Tools |
vegaro
left a comment
vegaro
left a comment
There was a problem hiding this comment.
I think this makes sense and should fix the issue. I don't have a lot of experience with recursive NSLocks, but I don't think there's any con on making the change right?
|
There are actually cons with turning a lock into recursive. The biggest one is that one can easily lose control of the lock and "leak" it everywhere. This is a big problem that we have had for a long time in AsyncDisplayKit/Texture. Go lang doesn't even have a built-in recursive lock out of principle. |
|
And btw, this fix (i.e making the lock recursive) doesn't work because re-entrant is not the root cause here. The root cause, as described here, involves 2 threads, a lock and a |
Introduces comprehensive unit tests for SynchronizedUserDefaults, covering deadlock scenarios, basic read/write operations, re-entrant and concurrent access. Also updates Atomic.swift to avoid deadlock by executing actions directly on the main thread.
JZDesign
changed the title
| // It didn't become unnecessary until iOS 12 and macOS 10.14 (Mojave): | ||
| // https://developer.apple.com/documentation/macos-release-notes/foundation-release-notes | ||
| // there are reports it is still needed if you save to defaults then immediately kill the app. | ||
| // Also, it has not been marked deprecated... yet. | ||
| $0.synchronize() |
There was a problem hiding this comment.
I'm not sure I agree that this decision is still necessary.
Checklist
purchases-androidand hybridsMotivation
resolves: #5729
resolves: #4137
Description
Modifies the "Atomic" class to accept an injected Lock, allowing the call site to include a recursive lock. Also checks if the thread is main and circumvents the lock when this is the case.
Big thanks to @denrase & @nguyenhuy for digging into this and even writing tests and solutions to look into.
@denrase has a PR here that removes atomic. I personally am not opposed to that. But I want other members of the team to be a part of that conversation.
Note
I am wanting to remove the use of Atomic, and have started doing that locally, but I can lock up UserDefaults on concurrent writes without the atomic wrapper. Concurrent reads is working just fine. That gives me pause. I haven't gotten to the bottom of it yet. But it only happens when I run my whole test class, not the individual tests.
Discussion
I believe that we should remove the lock around the UserDefaults. The one risk I see with that approach however is with concurrent writes potentially locking things up. I think that is a relatively low risk in comparison to what we are currently experiencing with an easily reproducible deadlock on read, considering that we do more reading than writing. This PR, as it currently sits works but seems like a duct tape patch for the underlying issue. We are simply checking if we're on the main thread and ignoring the lock in that case. We added a recursive lock, but it's actually of no benefit. If I'm remembering correctly, my testing of the concurrent reads and writes succeeded without the recursive lock. I can check again to be sure. But in the end, we either have the lock, with a main thread check on read and ensure sequential reads and writes with no deadlocks. Or no lock at all and can potentially get a locked up thread on writes.
Now… there is probably another 3rd option I haven't found yet. But these are the 2 I am currently aware of, and their tradeoffs.