Isolated synchronous deinit #2

I'm processing feedback from the first iteration:


So, in the updated proposal current behaviour will be preserved by default, and isolation will require explicit opt-in.

For actors, syntax would be isolated deinit {.

For GAIT, there are two options. One can similarly use attribute isolated to opt-in into inheriting global actor from the class. Alternatively one can explicitly repeat global actor declaration:

class MyView1: UIView {
    isolated deinit { ... }
}

class MyView2: UIView {
    @MainActor deinit { ... }
}

I think the first option is more preferable, because it follows DRY principle. So, it will be recommended in the proposal text, any updates to the Swift book should use it, and in case of ambiguity it will be chosen for fix-its.

It is an error to use isolated if there is no global actor isolation to inherit. It is an error to combine isolated attribute with nonisolated or global-actor isolation attribute.


I'm willing to push back on this. I think it is a premature optimization. I see value in having access to the task locals in the deinit. If object is referenced by several child tasks, you cannot know which one will make last release, so you cannot reliably use values set in child tasks. But as long as you are confident that object does not escape scope of the parent task, you can still reliably use values set by the parent task.

If task locals are used to inject logger/error tracker, it is easy to forget to reinject it in the deinit. This will cause errors occurring inside the deinit to be not visible in the dashboards. And if errors are rare and unexpected, both problems (error itself and broken monitoring) can remain undiscovered for a long time.

class ErrorLogger {
    @TaskLocal
    static var instance: ErrorLogger? = nil

    func log(_ error: Error) { ... }
}

@MainActor
class Resource {
    isolated deinit {
        shutdown()
    }
}

func shutdown() {
    do {
        ...
    } catch {
        ErrorLogger.instance?.log(error)
    }
}

And also note that performance cost for copying task locals occurs only when hopping is needed. If last release occurs on the right actor, no copying happens. Which I expect to be pretty common.

To support demand for blocking copying task-locals in isolated deinit, I can suggest to use an attribute:

@MainActor
class Resource {
    let errorLogger: ErrorLogger

    @detached
    isolated deinit {
        EventLogger.$instance.withValue(errorLogger) {
            shutdown()
        }
    }
}

Attribute @detached implies analogy between Task vs Task.detached. Which is a good analogy in regards to task-locals, but Task vs Task.detached also differs in priority inheritance. I don't recall seeing much demand for priority management for isolated deinit, so it might be better to use different attribute name to avoid overpromising.

If implementing blocking task-locals, I can also add public API for that, something like this:

public func withResetTaskLocalValues<R>(
    operation: () async throws -> R,
    file: String = #fileID, line: UInt = #line
) async rethrows -> R

And one could use this function to achieve blocking task-locals deinit as a functional requirement, but not for performance reasons:

// When hopping, task-locals are first copied
isolated deinit {
    // ... and then blocked
    withResetTaskLocalValues {
        ...
    }
}

As an alternative to @detached attribute, it is possible to provide a guaranteed optimization that recognises this pattern and blocks task-locals in the swift_task_deinitOnExecutor.


Async deinit is a big topic, I'm gonna make a separate thread for it.

6 Likes

Hi Nickolas! Super happy that you've picked this up again!

I agree with this as the recommended spelling for a GAIT's deinit. Basically isolated and nonisolated operate as "isolate to self (or not)" (though we had some discussions about the nuances there with @kavon recently), so it makes sense for an isolated deinit {} to mean the same when defined in a concrete actor or any other type annotated with @GlobalActor :+1:

The latter I guess does not really have to be banned... if that would just work? No strong feels on that.

It is an error to use isolated if there is no global actor isolation to inherit. It is an error to combine isolated attribute with nonisolated or global-actor isolation attribute.

Agreed as well, sounds good.

Relying on a task local to be present in deinit has me very concerned. We should perform some benchmarks to see the impact here.

I really would like to be able to recycle tasks for the purpose of such deinit (and similar things). So whatever we come up with here really should not prevent such this optimization in the future. It is not the optimization right now that I'm concerned about.

I had to re-read this a few times to see what you mean... Specifically you mean that if we're going to cause the deinit while executing on the right actor already, then you'd run the deinit in-place. Yeah then by accident we don't have to copy, that's true...

But once we get to async deinits, we'll always have to, because the Task{} we kick off to perform a deinit is unstructured, i.e. we don't await for its completion inside the scope where deinit was "caused", so we always must copy locals if we say they are guarnteed to be seen in deinit. Visualized:

func x() {
  let x = X()
  // Task { await x.deinit() }
  // no await on the deinit means that we must copy the task-locals
}

Perhaps it's worth writing up a matrix of all the cases and expected behaviors so we can see if we can come to an agreement here?

The "reset task locals" function can potentially be useful in any case; that's a nice thing we might want to consider. Implementation wise we can do this very nicely by inserting a tombstone into the chain so lookups simply stop searching once they notice it :+1:

2 Likes