[swift-dev] Thread safety of weak properties

[Mike Ash]

Hello, world!

First, congratulations on the whole open source thing. I'm really pleased to see how the team set it up and how well it's going. Blew away my expectations.

Anyway, on to the thing.

I was looking through the standard library's implementation of weak references, as one does, and noticed that loading a weak reference is not thread safe. I'm not sure if this is by intent or by omission. On one hand, loading a weak reference *looks like* just reading a stored property, which should be thread safe if not done concurrently with any writes. On the other hand, loading a weak reference is *actually* a potential mutation of that stored property, from its original content to nil, which one would not expect to be thread safe. It's clear that weak references are supposed to be thread safe with respect to the target object being destroyed, but less clear whether they're supposed to be thread safe for concurrent accesses of the same weak reference.

Is there an explicit intent as to whether loading a weak reference should be thread safe or not?

The root of the problem (or not-problem, if it's supposed to be this way) is in the swift_weakLoadStrong function in HeapObject.cpp. If two threads simultaneously enter this function with the same weak reference and object->refCount.isDeallocating() is true, the threads will race. This can result in calling swift_unownedRelease() twice resulting in a double-free or worse, or it could result in one thread calling swift_tryRetain() on a deallocated object.

If making this thread safe is desired, there are a couple of potential fixes.

One fix would be to just remove these two lines:

    swift_unownedRelease(object);
    ref->Value = nullptr;

This would cause the weak reference to never become nil in memory, and the target object's memory to stay allocated until the weak reference is destroyed or reassigned, but it would eliminate the potential for a race.

Another potential fix would be to add a lock to make this function thread safe. This could be done with low overhead by stealing a bit in ref->Value and using that bit as a spinlock. It pains me to think of adding a lock to this lovely lock-free code, but at least it would be specific to the individual reference.

I haven't filed a bug yet, since I didn't know if it's supposed to be like this or not. If a fix is desired, I'd be happy to file, and maybe make the fix too.

In case it's helpful, here is my test code which demonstrates the crash:

import Foundation

class Target {}

class WeakHolder {
    weak var weak: Target?
}

for i in 0..<1000000 {
    print(i)
    let holder = WeakHolder()
    holder.weak = Target()
    dispatch_async(dispatch_get_global_queue(0, 0), {
        let _ = holder.weak
    })
    dispatch_async(dispatch_get_global_queue(0, 0), {
        let _ = holder.weak
    })
}

Mike