[Second Review] SE-0371: Isolated synchronous deinit

Evolution Proposal Reviews
1

Hello Swift community,

The second review of SE-0371: Isolated synchronous deinit begins now and runs until August 6th, 2024.

This proposal was originally reviewed in 2022, when it was returned for revision. The author has now updated the implementation and proposal in response to the feedback provided by the Language Steering Group:

  • Originally, it was proposed that deinits would be isolated by default. In the current proposal, the behavior is opt-in via the isolated deinit spelling.
  • Originally, it was proposed that task locals would be copied when needed to maintain their values within the isolated deinit. In the current proposal, the behavior of task locals is left as an unspecified implementation details. Clients should not rely on task locals having any particular value in the body of an isolated deinit.

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to me as the review manager via the forum messaging feature or email. When contacting the review manager directly, please put "SE-0371" in the subject line.

Try it out

Toolchains with the feature enabled are available here:

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?

  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at swift-evolution/process.md at main ¡ apple/swift-evolution ¡ GitHub .

Thank you,

Freddy Kellison-Linn
Review Manager

13 Likes
2

Does this proposal really propose a “synchronous” deinit? The behavior is described thusly:

This proposal introduces a new runtime function which ensures that the body of an isolated deinit is running on the correct executor. If no switching is needed, then the deinit is executed synchronously. Otherwise, the task-less job is scheduled with the same priority as the current task/thread that released the last strong reference to the object.

My understanding of “synchronous” implies that code running on any arbitrary actor that knows it is holding the only reference to a value can deterministically guarantee that the value’s deinit has finished executing by the time the value goes out of scope. But A “task-less job” that is “scheduled” does not imply that the calling context actually waits for the job to complete.

In other words, is this code safe?

nonisolated let global: Atomic<Bool> = false

@MainActor struct Value {
  isolated deinit {
    global.store(true)
  }
}

nonisolated func foo() {
  assert(global.load() == false)
  do {
    let v = Value()
  }
  assert(global.load() == true) // does this proposal guarantee this?
}
2 Likes
3

I’m not closely following async proposals, but this is a Hyrum’s Law violation waiting to happen. Is it possible to block task locals, if they won’t be copied? As if it were always a fresh task?

8 Likes
4

In a previous review we had pushed for "clear always", which can be done, and was implemented at some point by the author here. A "clear" i.e. isolated deinits NEVER see any task local values has the benefit that at least it's consistent... but it also has the weird implication that normal deinits actually DO see task locals sometimes -- in the same way as the current proposal's isolated deinits, just depending on if they luckily deinitialized in "some" specific task.

There is a long discussion before this second review round about the task locals behavior here: [Sub-Pitch] Task-local values in isolated synchronous deinit and async deinit - #28 by johannesweiss

Though let me just call out the language steering group's request from that thread, as that's what informed the current proposal / implementation (link):

The thread also continues to basically acknowledge that deinits are not good for resource cleanup that isn't just memory anyway, with some good examples listed. As such... I think the "guarantee nothing specific" which is the same that normal deinit does is the least painting ourselfes into some corner approach from all of them.

It's up for discussion, that's what a review is for after all, but I wanted to share that this was discussed a lot already :sweat_smile:

// That's with my summary of the situation with task locals as "someone trying to shepard this proposal to completion" hat on, who has been following all the discussions.

5

+1

The proposal is well written and not only provides more convenience when dealing with isolated contexts, but also addresses a real ergonomic problem, at least on Apple platforms. When deriving from UIKit and AppKit classes, as described in the motivation, one currently has to work around these problems by using, for example, MainActor.assumeIsolated {}.

6

I'll let the proposal author @Nickolas_Pohilets reply in more depth, but I wanted to chime in and say that relying on this is incredibly brittle and you cannot rely on isolated deinits to have strict order because they're so contextual...

There's also some annoying interaction with "detect the right context" that we cannot implement in a better way because we're missing APIs to do so from e.g. dispatch (the recent checkIsolated proposal I worked on is related to this, so let me explain what's up with that).

Your example:

Okey, so this is a nonisolated synchronous function... It may not be executing on the main actor... and yeah then a new job has to be created, and won't be awaited on -- notice that it literarily can't be awaited on, we're in an sync function (it would not be awaited on in an async one anyway).

So no, this is not guaranteed.

You'd "get lucky" with such code:

@MainActor
func foo() {
  assert(global.load() == false)
  do {
    let v = Value() // lucky, inline synchronous run...
  }
  assert(global.load() == true) // got lucky
}

Because the implementation is... (In general we don't need to discuss implementation details, but let me bring this up here because it affects what kinds of semantics we're even able to implement):

  // If the current executor is compatible with running the new executor,
  // we can just immediately continue running with the resume function
  // we were passed in.
  //
  // Note that isCurrentExecutor() returns true for @MainActor
  // when running on the main thread without any executor.
  //
  // We always use "legacy" checking mode here, because that's the desired
  // behaviour for this use case. This does not change with SDK version or
  // language mode.
  if (isCurrentExecutor(newExecutor, Legacy_NoCheckIsolated_NonCrashing)) {
    return work(object); // 'return' forces tail call
  }

And we're specifically able to handle the following situation because or specialized logic to detect the main thread and assume it is the main actor:

@MainActor struct Value { ... } 

func foo() {
  DispatchQueue.main.async { 
    assert(global.load() == false)
    do {
      let v = Value()
    }
    assert(global.load() == true) // lucky case
  }
}

:warning: However... this cannot handle the following situation:

// assume SomeActor is ON a specific queue
@SomeActor struct Value { ... } 

func foo() {
  SomeActor.queue.async { 
    assert(global.load() == false)
    do {
      let v = Value() // isolated on same queue...
    }
    assert(global.load() == true) // NOT GUARANTEED
  }
}

One might hope that "hey, we're the exact queue that SomeActor is isolated to" so this could work the same as the previous main actor example right? The answer is sadly no, because SE-0424 custom isolation checking for SerialExecutor which the would make this possible... cannot ever return "false" in this isCurrentExecutor branch. We're unable to implement this API to detect the dispatch queue is "the same" isolation context, because there is no executor here at all, and dispatch does not offer an API to return false in such situation, but can only ever crash when the condition is false.

Therefore... such situations will NOT be able to take the fast-path and will have to enqueue a job.

It would be great to be able to implement optimizations based on the ability to compare the current to an expected queue, but there is no API or SPI to be able to do this today; and thus there are edge cases like this.

// Note: that assumeIsolated can do the right thing and can now detect the queue.async { queueActor.assumeIsolated {} } exactly because in the false case this method will crash.

The short version of this story is: The ordering semantics are very brittle and should only be seen as an optimization, rather than something to rely on. You should not be relying on ordering guarantees of isolated deinits as they're so highly contextual.

You can get lucky sometimes, or construct a very very specific situation that will work fine, but it's difficult and I don't recommend this pattern.

I also recommend this writeup by @johannesweiss about the exact same problem from a related thread: [Sub-Pitch] Task-local values in isolated synchronous deinit and async deinit - #28 by johannesweiss

Long story short: the only safe guaranteed way to do such patterns is still with ... { } blocks or specifically ~Escaping & ~Copyable types, where you can exactly predict where the deinit will happen.

2 Likes
7

I guess there's yet another optimization with default actors where they may get a chance to deinit inline, but again -- I would not count on specific behavior of ordering of those deinitializers...

8

In addition to @ktoso's excellent response I would just add that on the nomenclature front:

The use of "synchronous" should be understood narrowly here to just be explicit about the fact that the proposal does not cover a hypothetical deinit async feature: though the body of a deinit would run in the proper isolation domain, it cannot itself suspend during execution.

1 Like
9

It is synchronous in the sense that deinit async it out of scope of this proposal. So, no, proposal does not guarantee this.

1 Like
10

It is possible to block task locals. It is possible to copy them consistently. It is even possible to crash when accessing the value which was not set explicitly inside deinit body.

We just didn't have enough user input to make a well informed decision about this. So at this stage I think it is really the best strategy to keep behaviour undefined and refine it later if see sufficient evidence that this is an aspect important for the users.

1 Like
11

From reading the proposal, previous thread and along-side thread an task locals, I have an impression that deinit async (and I see in previous review also were "votes" for it), or any other explicit mark (isolated(unsafe)? makes not a lot of sense though) that this change will make an effect on how deterministicaly deinit is executed. IMO isolated in that case has less expressivity over async, but latter brings also the need to support other asynchoronous operations, which might be too much.

I also support this take on importance of documenting, as it might be not clear (especially to newcomers to the language) to understand why this feature exists and what aims to address.

Aside from that, +1.

1 Like
12

-1 from me.

My concerns remain the same as for the pitch and therefore I'll just replicate what I wrote then:

I have one main concern with isolated deinits: It introduces GC-style 'do it later' memory reclamation (*) without backpressure into the language.

This system works okay, if the garbage producer (i.e. the piece of code that lets the reference go to ref count 0) is slower as the garbage consumer (i.e. the actor's executor that'll run the isolated deinit). There is no backpressure which means that even if the garbage consumer is overwhelmed, the garbage producer will not be slowed down. That's common in GC'd systems but is usually not the case with ARC. In ARC, the deinit usually run synchronously and in line with the place that decrements the reference to 0. So assuming the backpressure works otherwise, ARC won't break it.
After introducing isolated deinit we could however get to a place where the garbage consumer cannot catch up with the garbage producer and we will start to uncontrollably accumulate memory, possibly until an OOM kill (**).

Some more information and discussion in and in response to my original comment on the pitch: [Sub-Pitch] Task-local values in isolated synchronous deinit and async deinit - #28 by johannesweiss

9 Likes
13

Just to be clear, we should review this proposal (synchronous, isolated deinits) in isolation. I very much doubt we'll ever see async in deinit. Maybe @John_McCall can comment on this.

Why don't I think that async in deinit will ever be a thing? I don't know how this is supposed to work. It's central and critical to Swift's async model that anything async is awaited by something (of course in an async context). But there are no awaits for references going out of scope and neither is there a guarantee that objects that use async deinit can only go out of scope in async contexts. This just feels impossible without completely undoing the model. @John_McCall please correct me if I'm missing something.

14

Poor choice of words by me, perhaps. What I meant is that isolated deinit in fact should be treated as if it would run asynchronously, since there can be a hop to the actor’s executor. And to me the most of the confusion is goes from that such deinit "quacks" like synchronous call, but in fact can be delayed compared to other code, so probably making it harder to reach for this tool in the language can make the feature both more obvious and less abused where it shouldn’t be. Something, that would tell developers think twice if that is what you need.

15

I also remain a -1 on this proposal. I understand the problem that we are trying to fix here for existing code that wants to migrate over to Swift Concurrency. Such code contains classes with deinits that are now forced to be actor isolated by e.g. @MainActor annotations. However, I agree with @johannesweiss here that by adding isolated deinit we are opening up a totally new can of worms by spawning unstructured tasks (taskless jobs are very similar) in the runtime that nobody owns. So far the only task that is ever spawned by the runtime/compiler is the one needed for the main entry point and IMO it should remain this way.

As @johannesweiss points out this can lead to severe security problems and open up remote denial of service attacks. Our current advise in the server ecosystem is to avoid deinits unless it is for managing memory. Personally, I think we would need to advise to never use isolated deinits in server library or application code since it would become impossible to spot where we are dropping the last reference to something that spawns the deinit job. If that happens in an attacker controlled path we just opened ourselves up for a denial of service.This goes against one of the core principles of Swift which is being safe by default.

5 Likes
16

While I agree with the downsides so far, isolated deinit is optional and not the default. As with other concurrency constructs, one must understand the implications. There are still many Swift apps outside of the server space, and a smooth transition path to Swift concurrency will certainly benefit its adoption for pre-existing complex code. Using isolated deinit for UIKit and AppKit classes that already perform late-stage memory reclamation does not change anything. I see isolated deinit as an additional tool on the path to strict concurrency, and would argue that no Swift newbie would simply adopt it because it is not the default.

1 Like
17

I concur with Franz.

If we are going to add isolated deinits, then please with extensive documentation from the get go. We should dissuade users from using this (alongside highlighting a few examples that this was designed for).

Tl;dr: -1 still. But if it gets in, with proper documentation please.

7 Likes
18

I don't want to bikeshed deinit async too much in this thread, but I've always envisioned it as being called with an implicit suspension point if the caller is an async function (and the compiler can statically prove it's safe to do so) or falling back to producing an unstructured-but-not-detached child task so that deinitialization is guaranteed to complete by the time the current task completes.

If the child task approach were too non-deterministic, we could require that a function that consumes or copies a value with an asynchronous deinit always itself be async, and only allow synchronous functions to borrow such values. If we approached it that way, then it would probably require something akin to ~Copyable and ~Escapable to convey that the type has this extra constraint. Maybe ~Synchronizable, I dunno.

Anyway, I don't want to derail this thread—I'd be happy to discuss further in another thread or in DMs. :slight_smile:

Regarding this proposal: isolated deinit needs to exist, but I don't love that the default is nonisolated. I understand we can't go breaking every class and move-only type in the Swift universe for the sake of this feature, but I wonder if changing the default would actually be that impactful…? (I admit I'm ignoring ABI considerations when I write that.)

19

+1, since proposed isolated deinit is an additive feature that is less invasive than 1st round review, and lack of 1. logging (e.g. naive memory leak check) and 2. cleanUp call in deinit are not practical for most of developers.

isolated deinit {
    print(“[deinit] \(self.name)”) // 1. Logging
    self.cleanUp() // 2. Clean up
}

BTW, according Non-Isolated Deinitialization and in previous conversation, it mentions to have some Sendable wrapper and pass to Task to call any additional methods after deinit.
While this suggestion already faces the fact that we still need to rely on “unstructured” Task, this approach is not feasible when considering about self.cleanUp() where clean up method belongs to self.

20

Sorry for the late late late reply on this, read the pitch thread and the proposal and I did not see the alternative to allowing Swift to follow’s Objective-C’s path of overriding release calls by adding required slots in vtables / wvtables discussed much in depth (even in the proposal)

What is the cost all classes would have to bear? How significant would it be?

If we decided that the cost was worth taking, what would be the other disadvantages? How would the proposal look?
Trying to understand that, aside from extra memory and bookkeeping costs, if for example the other concerns and “watch-out-for” items called out in the proposal would be gone or not? Would it be happy sunlit uplands aside from the programs taking a “bit more” memory and consuming a “but more” CPU resources but a much simpler mental model in that scenario?