Arguably, in your code (and mine before) you have published a self reference from deinit. When trying to print it (after it escaped deinit), you then hit a bad pointer (EXC_BAD_ACCESS on 0x1c) because it's a use after free. If you re-run your program in a loop, I wouldn't be surprised if the exact output changes.
As you said, we could (should!) catch this. The condition to catch isn't "retain a dying object" (that's legal) but that the reference count at the end of deinit is > 0 which clearly indicates that we've published self.
Said that, as Chris says ...
... this isn't specific to this proposal. The reason I bring it up is that I think relying on deinit may cause self publishing issues more frequently.
Example: [It hurts to type this]
class DBCursor: AsyncIteratorProtocol /* assuming no `cancel` at all */ {
func tearDownCursor() async throws {
// ...
}
deinit { // oops, and causes a UaF or a crash (once the Swift bug is fixed)
_ = Task.runDetached {
try? await self.tearDownCursor() // <- attempted self publish in `deinit`
}
}
Sure, the code is awful but could we really blame somebody for writing it if they're just given func tearDownCursor() async throws and deinit?
Yes, you are right. I would indeed the happiest with func cancel() async throws. If we can't get this for efficiency/complexity reasons, I am okay with having the synchronous func cancel(). At least I still know how to do my job being given a func cancel. Without any explicit cancel I really don't know if we can get everything to work.
Regarding the synchronous cancel: I have shown an example of async teardown with just a sync cancel at the end of this post ("Details" at the very end).
Also my bad example from above would totally work with a synchronous cancel. Yes, not how structured concurrency is meant (and likely inefficient). But hey, contrary to putting the code in deinit it would actually work:
class DBCursor: AsyncIterator /* with sync cancel, as proposed */ {
func tearDownCursor() async throws {
// ...
}
func cancel() { // works, not ideal (and allocates for the closure)
_ = Task.runDetached {
try? await self.tearDownCursor()
}
}
Obviously as you note, an async cancel would make everything ...
class DBCursor: AsyncIteratorProtocol /* with async cancel */ {
func tearDownCursor() async throws {
// ...
}
func cancel() async throws {
try await self.tearDownCursor()
}
}
... so much nicer indeed (but yes, does need somebody/something to call the cancel).
Yes, having the DBConnection own all the cursors' resources does work, we're essentially pushing the cleanup responsibility for the cursor up one layer (from cursor to connection).
There is one important ownership question though: DBCursor has to hold a strong reference to DBConnection because otherwise for row in makeDBConnection().request("SELECT foo FROM bar").makeCursor() wouldn't work because the DBConnection would no longer be directly referenced.
Essentially, the cursor has to retain the connection. But what do we do if the last cursor deinits? If we don't take any precautions, then DBConnection.deinit would now be invoked and also needs to finish synchronously (but still needs to communicate with the DB to tear down the cursor, which is async).
To work around this, we could have the connection create a (temporary) retain cycle with itself once it starts tearing down a cursor. And when it's done tearing down the cursor, it'd break that cycle [alternative: keep self alive in an async func]. That however leads to the next problem: If we're using also using ARC to manage the connection, the connection now needs to (asynchronously) tear itself down in the synchronous deinit. Yes, we can play the whole game again and say that the connection teardown is owned by a NetworkingSystem and DBConnection.deinit would just send a message to NetworkingSystem to tear the connection down. And then NetworkingSystem needs a tear down at some point... This feels like we're always pushing the responsibility for the cleanup up by one layer until we have no more layers.
Maybe we can make this "push the cleanup responsibility up" design work somehow but it seems very complex and error-prone. Also it does feel a little backwards to me given that DBCursor will need to be a class anyway (for the deinit if we don't have cancel).
Just in case I misunderstood your DBCursor sends message/sync calls DBConnection to tear down. In the "Details" I have a code sketch how i understood both options:
class DBCursor {
// assuming no `cancel`
let connectionActor: DBConnectionActor
let myCursorID = ... // need a unique ID here, can't use `self` or
// ObjectIdentifier(self) because needs to stay valid after
// `self` is deallocated. Likely provided in `init`
// from the `DBConnection`.
deinit {
// pull two variables out of `self` so we won't need `self` in
// detached task.
let connectionActor = self.connectionActor
let myCursorID = self.myCursorID
_ = Task.runDetached {
try await connectionActor.finishCursor(myCursorID)
}
}
}
- as "internally synchronised" class
class DBCursor {
// assuming no `cancel`
let connection: DBConnection
let myCursorID = ... // need a unique ID here, can't use `self` or
// ObjectIdentifier(self) because needs to stay valid after
// `self` is deallocated. Likely provided in `init`
// from the `DBConnection`.
deinit {
// problem here is: `DBConnection` needs to make sure it doesn't
// get `deinit`ed straight after this call.
self.connection.finishCursor(self.myCursorID)
}
}