I have run directly into the frustration of wanting an async deinit and an isolated deinit, but I'm -1. I think this special-cases the concurrency model in a way that will constrain further evolution (I can imagine a proposal for some cool isolation ergonomics proposal getting rejected with "ah but it would interfere with isolated deinits") and opens up the doors for even more usage of deinit for resource management, which my betters have pointed out isn't a good idea. Deinits are non-local reasoning, spooky action at a distance, and they don't need to be made more powerful. Lexical scopes provided by the humble withResource(action: (resource: Resource) -> ()) API pattern are a much saner way to do resource management that matches the language's emphasis on local reasoning, the whole reason we introduced the new concurrency model in the first place.
Would it be possible to change how this is spelled to make it clearer that this introduces potential unstructured concurrency? E.g. something like @unstructured isolated deinit? I don’t think the extra verbosity would be a problem for the few cases where this is really needed, but would also discourage its use in other places.
There is already a slot in VWT for deallocating deinit, which does the job. Adding new slots would affect all objects, even those that don't use the feature. The costs would include an indirect function call for every swift_retain()/swift_release(), which are ubiquitous. Extra slots would increase size of metadata in the binary. But probably the worst cost would be ABI compatibility concerns.
It does not address any of the concerns and "watch-out-for"'s, and maybe adds new ones.
So I think this alternative has only disadvantages.
This feature is intended to support deinit in objects with shared ownership, where lexical scopes don't apply. In particular for objects which are part of the UIKit/SwiftUI hierarchy.
most of the discussion in this review so far has revolved around using the feature for its “intended purpose”, that is, in situations where it is actually important to define where things run in a deinit. this is a silly observation, but perhaps not as silly as it might seem on the surface.
i primarily think about isolated deinit in terms of surpressing unhelpful compiler warnings, which will soon become blocking compiler errors that i am dreading having to devise workarounds for.
a cartoonish example might look like this:
actor A
{
private
var poolThatMustBeDrained:[ClientBootstrap]
deinit
{
if !self.poolThatMustBeDrained.isEmpty
{
fatalError("""
deallocated while `poolThatMustBeDrained` \
still contains bootstraps!!!
""")
}
}
}
the familiar villain in this sketch is ClientBootstrap, which is non-Sendable and therefore cannot be touched from within a deinit without this magical isolated deinit construct. ensuring that the deinit always runs isolated to the actor is one way to remedy the problem. but actually running the deinit in isolation is a non-goal - it is merely an overpowered means of silencing a zealous compiler.
i agree with many of the other reviewers in this thread that most types that actually do care about deinit isolation should not be doing cleanup in a deinit in the first place, and would probably be better-suited by a more deliberate life cycle API.
it’s possible that RBI in Swift 6 will undercut some of this motivation, but i have not accumulated enough experience using the RBI feature to say if this is true or not, largely because it is difficult to obtain Swift 6 toolchains for the platforms where i do the majority of my work.
Technically, when actor deinit is running, there still can be jobs running isolated to the actor and using ClientBootstrap. So compiler is correct in this case. And region-based isolation does not change anything. But it is a very edge case, which I still don't know how to reproduce.
+1 for me. This seems like a clear win, especially since it's opt-in.
I like to do defensive programming in deinit to make sure to close any file handles or network connections that I've previously opened. Yes, clients should use the class's close method instead, but they might not, and my application will be more reliable if I can guarantee that an object's external resources are disposed of when the object is dealloc'd.
Classes that are annotated @MainActor sometimes require a bunch of boilerplate to do this, e.g.:
deinit {
Task { @MainActor [publisher, connection] in
publisher.close()
connection.close()
}
}
This code would be simpler to write and understand if it were just:
isolated deinit {
publisher.close()
connection.close()
}
explicit
deinits in actors and GAITs.
It's a small thing, but please expand this acronym in the proposal document. While I figured out what it means, these texts are regularly referred to as documentation of the features they introduce, and they should be as clear as possible for future readers.
Very much agreed, this isn't a official term used by the Swift language and we should not slip it into proposals. It's easiest for editorial changes to PR directly. In this case I just prepared the PR right away though: Avoid confusing acronym in SE-0371 by ktoso · Pull Request #2538 · swiftlang/swift-evolution · GitHub -- needs language team approval to edit this though.
I haven't really followed this discussion, and haven't read the whole SE, but has it been considered to add a new thing and keep the existing deinit? Making sure to document the new thing with all its dangers and downsides?
Like e.g.
deinit {
// Perform usual sane cleanup
}
isolatedDeinitialization {
// Do stuff here at some point
// This can delay deinit indefinitely etc...
}
SE-0371 has been accepted with modifications; please see Freddy's post for a thorough discussion of the Language Steering Group's thoughts.
John McCall
Language Steering Group chair