mattmassicotte / ConcurrencyRecipes

Practical solutions to problems with Swift Concurrency
BSD 3-Clause "New" or "Revised" License
1.07k stars 33 forks source link

Priority Propagation #5

Open fonkadelic opened 6 months ago

fonkadelic commented 6 months ago

The recipe about structured concurrency mentions in "Solution #1: Use an unstructured Task":

Move the async calculation into a Task, and cache that instead. This is straightforward, but because it uses unstructured concurrency, it no longer supports priority propagation or cancellation.

I've did a bit research on this and i think priority propagation works even with an unstructured task inside an actor.

For my tests i've used this slightly modified version of the code example:

actor MyActor {
  private var expensiveValueTask: Task<Int, Error>?

  private func makeValue() async throws -> Int {
    try await Task.sleep(for: .seconds(1))
    return 0 // we'll pretend this is really expensive
  }

  public var value: Int {
    get async throws {
      // note there are no awaits between the read and write of expensiveValueTask
      if let task = expensiveValueTask {
        return try await task.value
      }

      let task = Task {
        print("Actor task priority begin", Task.currentPriority.rawValue)
        defer { print("Actor task priority end", Task.currentPriority.rawValue) }

        return try await makeValue()
      }

      self.expensiveValueTask = task

      return try await task.value
    }
  }
}

let actor = MyActor()

let task = Task(priority: .low) {
  print("Consumer 1 task priority", Task.currentPriority.rawValue)
  let value = try await actor.value
  print("Consumer 1 finished", value)
}

Thread.sleep(forTimeInterval: 0.5)

let task2 = Task(priority: .high) {
  print("Consumer 2 task priority", Task.currentPriority.rawValue)
  let value = try await actor.value
  print("Consumer 2 finished", value)
}

Thread.sleep(forTimeInterval: 5)
Consumer 1 task priority 17
Actor task priority begin 17
Consumer 2 task priority 25
Actor task priority end 25
Consumer 2 finished 0
Consumer 1 finished 0

I've simply used a Swift package with an executableTarget to run the code above

The priority seems to be in sync and gets escalated which brings me to the conclusion that priority propagation works as expected even in this scenario.

mattmassicotte commented 6 months ago

Thank you so much! I love that you actually did some work here. To be honest, I was mostly parroting some information I got from Swift folks on the forums from here: https://forums.swift.org/t/structured-caching-in-an-actor/65501

It is possible that I'm wrong. It's also possible things have changed. Or maybe there are subtleties we aren't quite accounting for. Perhaps all three!

I'm going to look more closely at this soon, I just wanted to acknowledge the issue and make sure you knew that I appreciated it.

fonkadelic commented 6 months ago

Looking forward to your findings!

What could be really helpful for this project is wrapping some of the code snippets from the recipes in a Swift package and setting up a test suite. This would demonstrate the usage but also chances are high that the tests will reveal if things have changed in an unexpected way.

mattmassicotte commented 6 months ago

Ok, so first of all, this rocks.

I'm not entirely sure how to reconcile this result with what the Apple people told me in that forum. But I think it's pretty clear that this does in fact propagate priorities. I suspect there could be some other things going on here that this test may not account for. But, at this point, I think it's warranted that the wording be changed.

I also really love the idea of putting together some tests. At worst, they will help to catch dumb typos and at best they can challenge assumptions just like this.

I'll use this issue to track updating wording. The test suite is a little more work, and I've captured that with #7 .