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
_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev
