[swift-dev] Thread safety of weak properties

[John McCall]

> On Dec 10, 2015, at 6:50 PM, Mike Ash via swift-dev <swift-dev at swift.org> wrote:
> 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 intent is that weak references do not need to be safe against read/write and write/write races.  They do need to be safe against read/destroy and write/destroy races (by “destroy”, I mean destruction of the object, not the weak reference).  I agree that they should also be safe against read/read races.

> 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.

Yes, you’re absolutely right, this is a bug in the current implementation; good catch!

> 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.

That would be great.

There’s actually a higher-level semantic bug in this code, which is that we really do want weak references to make the same deallocation guarantees as Objective-C if possible.  That is, while we’re comfortable with the idea of an unowned reference pinning some memory until the unowned reference is destroyed, weak references should really allow the object’s memory to be more-or-less immediately returned to the general pool.  It was always our intention to revisit the implementation and do this.

It would be acceptable to increase the inline size of weak references if that makes this more performant.

John.