Weak captures in sendable/sending closures

Evolution Discussion
, ,
1

Consider the following example:

final class X: Sendable {
    func bar() {}
}

func doIt(_ block: () -> Void) {
    block()
}

func foo() {
    let x = X()
    doIt { [weak x] in
        Task { // error: passing closure as a 'sending' parameter risks causing data races between code in the current task and concurrent execution of the closure
            x?.bar() // note: closure captures 'x' which is accessible to code in the current task
        }
       // not needed, just to check what is going on
        x = nil // ok, confirms that x is mutable
    }
}

It seems that x is captured as a variable, which causes an error in isolation checking and allows x to be mutated.

Which kinda makes sense, because Swift does not allow weak let:

weak let y = x // error: 'weak' must be a mutable variable, because it may change at runtime

But understanding this does not help me much. I did not want to be able to modify the capture and did not express any intention for that, so for practical purposes this is a false positive from the isolation checker.

There is a workaround for that - to wrap it into a struct:

struct WeakX {
    weak var x: X?
}

func foo() {
    let x = X()
    let weakX = WeakX(x: x)
    doIt {
        Task {
            weakX.x?.bar()
        }
    }
}

And compiler is quite happy with immutable structs containing weak references, even though they also "may change at runtime".

It is probably possible to mitigate this with some hackery specific for closure captures, but maybe it is worth to reconsider the whole ban on weak let? WDYT?

2

The closure itself is not declared Sendable, which is why you get a compiler error trying to send a non-sendable. I do not see this as a false positive (or an error in isolation). Updating the relevant following code compiles without error.

func doIt(_ block: @Sendable () -> Void) {
    block()
}
3

This does fix the error, but I don't understand why. Now it looks like a data race to me:

func doIt(_ block: @Sendable @escaping () -> Void) {
    Task.detached(operation: block)
}

func foo() {
    let x = X()
    doIt { [weak x] in
        Task {
            x?.bar() // Read
        }
        x = nil // Write
    }
}

Read and write can happen concurrently, can't they?

4

Okay, let's see... I am not 100 % certain, but there's a couple of things:
First off: X is Sendable as it has no mutable state (no state at all, in fact), so there's not really any reads or writes happening to it. So no warnings expected, no encountered, we're good.

Next, capturing its instance in the closure passed to doIt: It's not just Sendable, but also not used after that line, so it could be treated as sending anyway. Not sure what takes precedence here, Sendableility or sending, but that doesn't really matter. Furthermore, it's captured weakly, so the closure does not take ownership.
That does technically introduce a race between it and its calling context, but only in the sense that if ´foo´ finishes before the closure runs, it can "vanish", but that is safe in the end. No warnings expected, none found.

The closure then immediately starts another unstructured task that will capture x again. I'm not sure if it loses its weakness by this, that's an interesting one. I assume no, at least it is still an optional.

There are no concurrent reads or writes happening: You assigning nil to the "middle" x is not a "write" in the sense that you manipulate the state of x. If that happens before the inner-most task calls bar on the instance, the expression resolves to nothing, but even if it's the other way around, the method doesn't do anything, there's no "read" in any way that matters.

You might think "But what if the nil-assignment happens during the execution of bar?" I think in that case the function itself has "ownership" of the instance (via the self parameter), so the fact that the variable it was called on "becomes nil" doesn't matter - the call finishes before the object is deinitialized. At least I believe that's how it could be phrased.

Lastly, I think the deinitializers of Sendable types are by now concurrency safe, so there's also that. Not that it matters, as you do not have any state in the type anyway...

5

Reading the SIL, I see that foo() creates a box for storing mutable weak reference. Reference to the box is then captured in both closures.
x = nil writes to the box, x? reads from the box. IIUC, that's a data race.

IIUC, this is equivalent to the following:

func foo() {
    let x = X()
    weak var y = x
    doIt {
        Task {
            y?.bar()
        }
        y = nil
    }
}

But here race is detected.

1 Like
6

It is racy, this crashes:

final class X: Sendable {
    func bar() {}
}

func doIt(_ block: @Sendable @escaping () -> Void) {
    Task.detached(operation: block)
}

func foo() {
    let x = X()
    doIt { [weak x] in
        Task {
            x?.bar()
        }
        x = nil
    }
}

Task.detached {
    for _ in 0..<1000000 {
        foo()
    }
}

try await Task.sleep(for: .seconds(10))

But I honestly did not know, that weak vars were assignable.

EDIT: Swift 5.10 could catch this: error: reference to captured var 'x' in concurrently-executing code.

2 Likes
7

That code doesn't crash on my machine (6.13.5-arch1-1) with Swift 6 language mode (and 5.10) or in godbolt.

I'll explain the code from Swift's perspective.

func doIt(_ block: @Sendable @escaping () -> Void) {
    Task.detached(operation: block)
}

func foo() {
    let x = X() // create an immutable reference type that is Sendable
    doIt { [weak x] in // create a detached task, executing a Sendable and escaping block of code, weakly referencing the immutable reference of `x`
        Task {
            // possible crash (bad pointer dereference)
            //x?.bar() // If weak reference x exists, call its "bar()"
            if let x { // workaround
                x.bar()
            }
        }
        x = nil // Assign the weak reference x to nil
    }
    // destroy x
}

Assigning a weak reference to nil doesn't modify the original reference. My understanding of this code is that the strong reference of variable x is destroyed before the detached task is executed (due to Swift optimizations, lifetime of the program, how it is scheduled, or something else).

Even if the detached task executed before the strong reference is destroyed, it is working with a weak reference to an object that is Sendable which means it is safe to be passed around. X doesn't have any mutable variables (or non-sendable ones) and the x variable is immutable, allowing it to be Sendable.

It doesn't matter what bar() does in this example, as the context in which it is called allows for it to be handled safely.

1 Like
8

Indeed. What does it modify instead? Where else is that location used?

9

It modifies the local variable that holds the reference. The x in the closure is like a local variable that points to the same instance as the x in the foo method. (If X were a value type things become a little complicated as it basically moves to the heap, but since it's a reference type here let's ignore that for now.)
Since it is weak it does not increase the reference count of the instance, so once foo returns (which might happen before the unstructured tasks do) and its local variable x gets destroyed, the instance's reference count reaches 0 and it gets deinitialized. The nice thing is that the x inside the first task (the detached one started by doIt) is weak, so this variable x now is nil. The same is true for the x in the inner-most task (that you capture implicitly). When you assign nil to the x in the detached task you might just achieve it becoming nil earlier.

I cannot test it right now, but am surprised something could crash here, at least not due to the assignment of nil...

This seems weird to me, as if let x { ... should be the same as the x?... maybe I was wrong and the instance can get deinitialized while bar is running? Then again, that's an empty function anyway... hm...

They have to be as the object they're referencing may become nil. This all just makes me wonder whether all implications in regards to concurrency and sendability have been caught here... but I am hesitant to assume the original code should give a warning or error... weird.

10

For me, the if let x does not change anything, it still crashes.

1 Like
11

I can confirm that it does indeed crash, but in the worst way: non-deterministically. Sometimes it crashes, sometimes not. And in some cases it does indeed blame the line where nil is assigned to x, but I think that may be a side effect from the boxing that is done for the capture somehow?

For what it's worth, I'm not sure if

is the exact same, as I don't know if y is captured in the same way as just using [weak x]... but I am not good enough with godbolt or SIL to judge that. I do notice that capturing y weakly again with [weak y] gets rid of the warning and reduces the chance of it crashing! (It still does, but on first glance not as often...)

Conclusion so far: Something here is definitely buggy (contrary to my first guess)! We might need some more input from folks?

12

Why not just re-capture it in the Task?

func foo() {
    let x = X()
    doIt { [weak x] in
        Task { [weak x] in
            x?.bar() // okay
        }
    }
}
13

That indeed seems to work (i.e. not crash), at least it hasn't done so in several tries now. It even works when you capture x without the weak keyword, i.e. just [x]!

However, I'd argue that it's still a bug, because the implicit capture should be the exact same. At least the compiler still realizes that x in the innermost task is a var and not a let (as a weakly captured variable should be) and does not complain about this:

func foo() {
    let x = X()
    doIt { [weak x] in
        Task {
            x?.bar() // sometimes crashes
            x = nil // okay, i.e. the task understands it's mutable
        }
        x = nil
    }
}

This makes me believe the bug resulting in the crash is that the weak is lost in the innermost task, it becomes a plain var x: X? and is not automatically set to nil when the instance this reference points to is deallocated from the enclosing task. When you explicitly capture it, with or without weak, it is safely (in the concurrency-relevant sense) set to nil, so we do not get a crash...

I also played around a bit with re-naming the captures and was not able to reproduce any kind of crash:

func foo() {
    let x = X()
    doIt { [weak y = x] in
        Task { [z = y] in // or [weak z = y] or [weak z = x]
            z?.bar()
//            z = nil // only works if you use `weak` as expected!
        }
        y = nil
    }
}

It really seems to be only a problem if you capture x weakly first and then, for the inner-most task, implicitly.
This seems to be a bug, I would expect the implicit capture to work exactly like an explicit one, whether with or without weak.
Concerning the original question about a data race I don't believe there is one, it should be legal to capture a Sendable reference type weakly.

Should we file a bug report about this?

14

I'll file the bug, and maybe even try to fix if time allows.

EDIT: Reported.

Meanwhile, can we get back to the discussion of the original question:

Does banning weak let actually make sense?

15

I would say yes, at it was never a legal declaration in the first place (at least not since I learned swift). Every weak variable declaration always had to be mutable, the current quickfix even explains it as such:

'weak' must be a mutable variable, because it may change at runtime

And obviously this is true whether we're in the context of concurrency or not.

The "weird" side effect of every weak variable being mutable is that we can also explicitly set it to nil. Forbidding this, be it only for captures in closures or more generally strikes me as overly strict and potentially more complex (it cannot "really" be immutable, but "looks like it"?).

More interesting is the question how already weak references are captures (see above) and how this plays out over isolation context switches...

16

It had to, but IIRC, that's a pretty artificial restriction. Swift is perfectly happy with let declarations of structs containing weak reference.

Native Swift weak references are reference-counted references to side table. When object dies they don't actually change and keep pointing to a zombie side table. ObjC weak reference do change, but in a thread-safe manner, so for isolation-checking purposes they are equivalent to immutable.

So technically, it seems to be possible to lift this limitation.

17

Sure, that's "weird" in the sense that something in an "immutably declared" value is mutated, but that's, imo, less confusing than allowing weak let. I don't see any benefit from preventing someone to explicitly set a (locally) declared weak variable to nil and only allow the runtime to mutate it should the object it points to be destroyed (which I assume is what you'd envision a weak let to actually function like). In both cases, any following code needs to be able to handle the nil-case.

I admit I lost you there. I do not know how weak is implemented internally in Swift and how it differs to the Objective-C ways.
But at the usage site, a weak variable just "suddenly" becomes nil when the underlying object disappears (because whatever else had retained it gives up ownership). Conceptually, that's why it is a var, I guess, even if that is a design decision of the language.
It is indeed, with concurrency, important to ensure that this is isolation-safe (i.e. what happens if the owning object releases the instance while the weak reference exists in a different isolation context), but I would assume that sendability ensures that (hopefully).
If not, that's a bug.

Allowing weak let, however, would not resolve this, as I understand you, it would only disallow the explicit mutation wherever the variable was declared.

In any way, I think weak let would warrant its own post/pitch as that's not (just) something concerning sendable or sending closures. I'd personally be against it, but that is, of course, an opinionated statement. :smiley:

18

Your assumption is correct.

There is a difference between the two. IIUC, nilifying on destruction is thread safe and can co-exist with other accesses to the weak variable. But explicitly setting is not. If two threads are trying to explicit set a weak variable - this is a data race.

So weak let is safe to be shared between multiple isolation domains, and it can be used in Sendable classes and @Sendable closures. But weak var is not.

19

I am not sure I follow. The weak captured variable is not shared itself, the instance it references is. Now the originating context has a (local) variable that references the instance and the "target" context has its own variable that references the instance as well (wrapped in an optional and without increasing its retain count).

Now I see that the weak variable is, in a sense, also influenced by the originating context, because if the original reference goes out of scope, the retain count is decreased and (assuming no other object has a strong reference) the instance gets deallocated, which in turn causes the weak variable to become nil.

If this is not done in an isolation-safe way, we have a problem, agreed. But setting the weak variable to nil is not the only thing that would be problematic here, since the deallocation is caused in a different context, i.e. asynchronously to the second context. So even if the variable were immutable to the declaring scope, if it becoming nil could happen in a non-thread safe way, every other mutation it is involved in (e.g. simply dereferencing it to access state in the instance) would also be problematic.

To reiterate: I'm not saying there is no problem, I don't know enough about the details when a Sendable object is deallocated and how and when that affects any references in different isolation contexts. But if there is a problem with this, I have my doubts that simply preventing an explicit nil-assignment would solve that by itself.
Unfortunately I don't know who could shed more light onto this?