[Second Review] SE-0371: Isolated synchronous deinit

Evolution Proposal Reviews
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