From eb116bef1e0524f7b77b171a21eeb094bd224ef5 Mon Sep 17 00:00:00 2001 From: Twig Date: Fri, 10 Feb 2023 13:19:29 -0500 Subject: [PATCH] Fixes index math being wrong in multi-threaded scenarios --- Sources/Persister/Utilities/LRUCache.swift | 16 ++-- .../Utilities/SynchronizedArray.swift | 83 +++++-------------- Tests/LRUCacheTests.swift | 15 ++++ 3 files changed, 42 insertions(+), 72 deletions(-) diff --git a/Sources/Persister/Utilities/LRUCache.swift b/Sources/Persister/Utilities/LRUCache.swift index 4c7ee09..3263134 100644 --- a/Sources/Persister/Utilities/LRUCache.swift +++ b/Sources/Persister/Utilities/LRUCache.swift @@ -30,11 +30,11 @@ public final class LRUCache { /// - date: The date at which the item is considered expired. If `nil`, the item will never expire. public func write(_ item: Value, for key: Key, expiresOn date: Date?) { - if let index = keysOrderedByRecentUse.firstIndex(of: key) { - moveKeyToFrontOfList(key: key, atIndex: index) + if keysOrderedByRecentUse.contains(element: key) { + moveKeyToFrontOfList(key: key) backingStoreDictionary[key] = item } else { - keysOrderedByRecentUse.insert(key, at: keysOrderedByRecentUse.startIndex) + keysOrderedByRecentUse.insertAtFront(key) backingStoreDictionary[key] = item } @@ -61,8 +61,8 @@ public final class LRUCache { return nil } - if let index = keysOrderedByRecentUse.firstIndex(of: key) { - moveKeyToFrontOfList(key: key, atIndex: index) + if keysOrderedByRecentUse.contains(element: key) { + moveKeyToFrontOfList(key: key) } return value @@ -99,9 +99,9 @@ public final class LRUCache { } } - private func moveKeyToFrontOfList(key: Key, atIndex index: Int) { - keysOrderedByRecentUse.remove(at: index) - keysOrderedByRecentUse.insert(key, at: keysOrderedByRecentUse.startIndex) + private func moveKeyToFrontOfList(key: Key) { + keysOrderedByRecentUse.removeAll(where: { $0 == key} ) + keysOrderedByRecentUse.insertAtFront(key) } @objc private func didReceiveMemoryWarning() { diff --git a/Sources/Persister/Utilities/SynchronizedArray.swift b/Sources/Persister/Utilities/SynchronizedArray.swift index e5f4df3..64a53f4 100644 --- a/Sources/Persister/Utilities/SynchronizedArray.swift +++ b/Sources/Persister/Utilities/SynchronizedArray.swift @@ -12,30 +12,13 @@ import Foundation final class SynchronizedArray { private var storage: [Element] = [] private let queue: DispatchQueue = DispatchQueue(label: "com.lickability.synchronized-array") - - /// The starting index of the array. - var startIndex: Int { - get { - var index: Int! - - queue.sync { - index = storage.startIndex - } - - return index - } - } - + /// The count of the array. var count: Int { get { - var count: Int! - - queue.sync { - count = storage.count + return queue.sync { + return storage.count } - - return count } } @@ -56,29 +39,12 @@ final class SynchronizedArray { } } get { - var element: Element! - - queue.sync { - element = storage[index] + return queue.sync { + return storage[index] } - - return element } } - /// The first index of the provided element. - /// - Parameter element: The element to find the index of. - /// - Returns: The index of the element, if it exists within the array. - func firstIndex(of element: Element) -> Array.Index? { - var index: Int? - - queue.sync { - index = storage.firstIndex(of: element) - } - - return index - } - /// Appends a new element to the end of the array. /// - Parameter newElement: The element to append. func append(newElement: Element) { @@ -87,41 +53,21 @@ final class SynchronizedArray { } } - /// Inserts the specified element at the index. + /// Inserts the specified element at the front of the array. /// - Parameters: /// - newElement: The element to insert. - /// - index: The index to insert at. - func insert(_ newElement: Element, at index: Int) { + func insertAtFront(_ newElement: Element) { queue.sync { - storage.insert(newElement, at: index) + storage.insert(newElement, at: storage.startIndex) } } /// Removes and returns the last element of the collection. /// - Returns: The last element of the collection. func popLast() -> Element? { - var element: Element? - - queue.sync { - element = storage.popLast() + return queue.sync { + return storage.popLast() } - - return element - } - - - /// Removes and returns the element at the specified position. - /// - Parameter index: The index of the element to remove. - /// - Returns: The element that was removed. - @discardableResult - func remove(at index: Int) -> Element { - var element: Element! - - queue.sync { - element = storage.remove(at: index) - } - - return element } /// Removes all elements from the array. @@ -138,4 +84,13 @@ final class SynchronizedArray { try storage.removeAll(where: shouldBeRemoved) } } + + /// A function that determines if the array contains the provided element. + /// - Parameter element: The element check to check for. + /// - Returns: Returns a `Bool` signalling if the array contains the provided element. + func contains(element: Element) -> Bool { + return queue.sync { + return storage.contains(element) + } + } } diff --git a/Tests/LRUCacheTests.swift b/Tests/LRUCacheTests.swift index e7dd1fa..3652ff9 100644 --- a/Tests/LRUCacheTests.swift +++ b/Tests/LRUCacheTests.swift @@ -71,4 +71,19 @@ class LRUCacheTests: XCTestCase { } } } + + func testMovingLastKeyDoesntCrash() { + let cache = LRUCache(capacity: .limited(numberOfItems: 3)) + let queue = DispatchQueue(label: "com.lickability.lrucachetestsqueue", attributes: [.concurrent]) + + for _ in 1...100000 { + cache.write("1", for: "1", expiresOn: nil) + cache.write("2", for: "2", expiresOn: nil) + cache.write("3", for: "3", expiresOn: nil) + + queue.async { + cache.removeItem(for: "2") + } + } + } }