Making a generic NSMapTable replacement written in Swift thread-safe

This is a follow-up to this question.

While discussing some details about the code I posted there, I came upon a problem with thread-safety. After searching and trying different things, I reached a potential solution that I now present here. This worked without any problems during my tests, though since thread-safety is not easy, I'd like to ask: is this implementation genuinely thread-safe?

The thread-safety problem was the following: keys on the dictionary can be deallocated on any thread. That means, when a key disappears, the callback block can also be called on any thread. That in turn means the dict var can be changed while it is being accessed.

My solution was to use dispatch_sync to coordinate all reads/writes, including the one happening inside the callback block from DeallocWatcher.

```
public class WeakKeyDictionary {

private var dict = SynchronizedValue(value: Dictionary, V>())
public var block: (V)->() = { _ in }

public init() {}
public init(dictionary: Dictionary) {
for (k, v) in dictionary {
setValue(v, forKey: k)
}
}

public subscript(key: K) -> V? {
get { return valueForKey(key) }
set { setValue(newValue, forKey: key) }
}

public func valueForKey(key: K) -> V? {
return dict.get { $0[HashableWeakBox(key)] }
}
public func setValue(newValue: V?, forKey key: K) {
let hashableBox = HashableWeakBox(key)

if let value = newValue {
let watcher = DeallocWatcher { [weak self] in
if let me = self {
if let v = me.syncedRemoveValueForKey(hashableBox) {
me.block(v)
}
}
}

objc_setAssociatedObject(key, unsafeAddressOf(self), watcher, objc_AssociationPolicy(OBJC_ASSOCIATION_RETAIN_NONATOMIC))
dict.access { $0[hashableBox] = value; return }
}
else {
objc_setAssociatedObject(key,

I appreciate that this is an old question, but a few thoughts. First, I might advise updating SynchronizedValue with modern GCD API. For example:
public class SynchronizedValue {
private let serialQueue = DispatchQueue(label: "…")
private var _wrappedValue: Wrapped

public init(wrappedValue v: Wrapped) { _wrappedValue = v }

public var wrappedValue: Wrapped {
get { serialQueue.sync { _wrappedValue } }
set { serialQueue.sync { _wrappedValue = newValue } }
}
}


Notes on the above:

• The contemporary GCD syntax.

• The sync method automatically returns whatever value the closure returns, so you do not need to capture the result in local variable, like you do in the get(action:) implementation. Just return the result of sync (or, because of SE-0255, you can even omit the return keyword).

• I might move the get/set methods into accessors for the wrappedValue. (This convention of wrappedValue also means that you can make this a @propertyWrapper if you would like.)

Note, the GCD serial queue is a nice, simple pattern. Some might advise a reader-writer pattern (which can be a little faster, but often introduces its own issues). Generally, if performance was important, I might jump directly to a lock, e.g., NSLock:
public class SynchronizedValue {
private let lock = NSLock()
private var _wrappedValue: Wrapped

public init(wrappedValue v: Wrapped) { _wrappedValue = v }

public var wrappedValue: Wrapped {
get { lock.withLock { _wrappedValue } }
set { lock.withLock { _wrappedValue = newValue } }
}
}


Or an OSAllocatedUnfairLock:
import os.lock

public class SynchronizedValue: @unchecked Sendable {
private let lock = OSAllocatedUnfairLock()
private var _wrappedValue: Wrapped

public init(wrappedValue v: Wrapped) { _wrappedValue = v }

public var wrappedValue: Wrapped {
get { lock.withLock { _wrappedValue } }
set { lock.withLock { _wrappedValue = newValue } }
}
}


You ask:

is this implementation genuinely thread-safe?

It is thread-safe insofar as you successfully prevent parallel access to the underlying dictionary.

But, two caveats:

-
There is a potential race between the deinit of DeallocWatcher and the adding of new values. Consider:
// remove strong reference to a key of something already in the map table; this will result it the map table’s weak key to be (eventually) deallocated
let firstKey = savedKeys.removeFirst()

// create a new key with the same identifier
let newKey = SampleKey(id: firstKey.id)

// add new value to the weak-to-strong map table
let value = SampleValue(…)
mapTable.setValue(value, forKey: newKey)

// make sure to save a strong reference to this new key
savedKeys.append(newKey)

// report results
print("Removed ", firstKey)
print("Added ", newKey, value)


If I remove the last strong reference to a key and then create a new key instance that is equal to the first one, the DeallocWatcher will be called after the new value was added, it might be unable to differentiate these two separate key instances, resulting in both being removed from the dictionary.

This is admittedly a contrived example, but it illustrates the subtle race between keys being deallocated and the eviction of the items from the broader dictionary when the last strong reference to this weak property is removed.

-
In a broader thread-safety observation, the underlying dictionary is not necessarily the right level for the synchronization. E.g., what will happen if you are iterating through the keys and values of the dictionary and another thread comes in and removes a key or adds a key. You might see the same value emitted twice, or see a value omitted, or get a nil value, etc.

If you are using this as a cache (where you are just looking for hits or misses), for example, that might not be a (serious) issue, but it is more problematic if you are iterating through the collection (which the presence of keys and values arrays suggest was your intent).

I might suggest eliminating the separate values array (as that is inconsistent with the concept of a collection that might be mutated in parallel). Or, personally, I would entirely eliminate these arrays and simply expose an interface like the access method that is currently buried in the SynchronizedValue object. The synchronization is not well suited at the subscript accessor level, but at a higher level of abstraction. You generally want to synchronize at the “ok, let me do a bunch of some stuff with the dictionary” level. (Yes, we see people share “thread-safe dictionaries” with all the dictionary-like interfaces on Stack Overflow, but, that is generally a mistake. I understand the intuitive appeal of the approach, but it is generally misguided, IMHO.)

Now, you only asked about the thread-safety question, but here are a few observations on the whole weak-to-strong map table concept:

-
It should be noted tha

StackExchange Code Review Q#85819, answer score: 3