Make ActorQueue tasks execute on the target actor's execution context #9

dfed · 2023-01-16T00:57:43Z

This PR updates the ActorQueue API to make tasks sent to the queue to execute within the target actor's context. This API change is a breaking one, and so we'll be bumping our version to 0.2.0 as part of this change.

Similarly to the benefits discussed in #6, the benefits of running tasks on the target actor's context are:

Multiple interactions with a single actor-conforming type can be executed without requiring multiple suspensions.
Only interacting with async-decorated properties and methods on a given actor requires the await keyword, enabling the casual reader to distinguish which tasks will execute synchronously and which may suspend.

These benefits are particularly important on an ActorQueue, where suspension can lead to interleaving of enqueued work.

The downside of this API is that avoiding a retain cycle between an actor queue and the target actor requires the use of an unowned reference and some force unwrapping. See comments below.

Note that this PR is built on top of #8 to avoid conflicts.

codecov · 2023-01-16T01:04:33Z

Codecov Report

Merging #9 (1c8cab5) into main (b073271) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main        #9   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          703       752   +49     
=========================================
+ Hits           703       752   +49

Impacted Files	Coverage Δ
Sources/AsyncQueue/FIFOQueue.swift	`100.00% <ø> (ø)`
Sources/AsyncQueue/ActorQueue.swift	`100.00% <100.00%> (ø)`
Tests/AsyncQueueTests/ActorQueueTests.swift	`100.00% <100.00%> (ø)`

dfed · 2023-01-16T01:16:42Z

Sources/AsyncQueue/ActorQueue.swift

-    public func async(_ task: @escaping @Sendable () async -> Void) {
-        taskStreamContinuation.yield(task)
+    public func async(_ task: @escaping @Sendable (isolated ActorType) async -> Void) {
+        taskStreamContinuation.yield(ActorTask(target: target!, task: task))


Using force unwraps is fine here given the strict instructions for how an ActorQueue should be utilized:

An ActorQueue's lifecycle should not exceed the lifecycle of its target

An ActorQueue's target should be set in the target actor's init

Therefore, the target will always be available if the API is properly utilized.

The other reason I opted to force unwrap is there is no good way to surface a "target is nil" error to the call-site without complicating the API. We could return a @discaradableResult Bool and return false if guard let target fails (likely the cleanest of our options), or we could make these APIs throw an surface an error when the target is nil... but, given the expected API contract the target should never be nil, at which point surfacing these kinds of errors in a non-crashing way to consumers isn't really helpful imo.

dfed · 2023-01-16T01:27:40Z

Sources/AsyncQueue/ActorQueue.swift


-    private actor ActorExecutor {


Now that we're explicitly targeting a specific actor, we no longer need another actor type! The code previously on ActorExecutor moved down into an extension on Actor below.

dfed · 2023-01-16T01:28:19Z

Sources/AsyncQueue/ActorQueue.swift

-                    // This works around a bug when compiling with Xcode 14.1: https://github.com/apple/swift/issues/62503
-                    _ = void


We no longer need this since we're sending self as a parameter of the task

dfed · 2023-01-16T07:45:51Z

README.md


-```swift


This code block didn't add as much as I wanted it to, and with the new API rev on ActorQueue it was difficult to showcase the ActorQueue API in a similar manner without breaking the ActorQueue's guidelines (i.e. we couldn't show a concise, free-form actor queue without adopting the queue execution context outside of an actor's init).

I don't think we lose much by deleting this code sample, since our in-code documentation explains everything explained here.

bachand

Love this @dfed . Great work iterating and coming to such an elegant solution 🏆

bachand · 2023-02-12T20:46:45Z

README.md

            count += 1
-            return count
+            let incrementedCount = count
+            XCTAssertEqual(incrementedCount, expectedCount) // often fails


I always think it's good to pass through file and line to helper functions that call XCTAssert* functions but I also see value in a simple README.

Curious why we need/want to change to this version of the code that no longer returns the new count.

Similar comment for other code examples.

Curious why we need/want to change to this version of the code that no longer returns the new count.

In this example it doesn't matter much, but in later examples where we have code like the following it becomes necessary and I didn't want to change examples too much:

for iteration in 1...100 { counter.incrementAndAssertCountEquals(iteration) }

If we used an increment that would return, we'd end up with:

for iteration in 1...100 { let count = await counter.increment() XCTAssertEqual(count, iteration) // this test will succeed since we `await` the return value of `increment` each time }

bachand · 2023-02-12T20:51:28Z

README.md

        })
    }
+    // Wait all enqueued tasks to finish.


bachand · 2023-02-12T20:51:45Z

README.md

        })
    }
+    // Wait all enqueued tasks to finish.


Suggested change

// Wait all enqueued tasks to finish.

// Wait for all enqueued tasks to finish.

?

🤦‍♂️ yes. Fixed up in a4871ad

README.md

bachand · 2023-02-12T20:55:31Z

README.md

 }
 ```

-### Sending ordered asynchronous tasks to Actors
+FIFO execution has a key downside: the queue must wait for all previously enqueued work – including suspended work – to complete before new work can begin. If you desire new work to start when a prior task suspends, utilize an `ActorQueue`.


Nice this is helpful.

bachand · 2023-02-12T21:21:53Z

Sources/AsyncQueue/FIFOQueue.swift

@@ -94,7 +94,7 @@ public final class FIFOQueue: Sendable {
    }

    /// Schedules an asynchronous throwing task and returns after the task is complete.
-    /// The scheduled task will not execute until all prior tasks have completed.
+    /// The scheduled task will not execute until all prior tasks – including suspended tasks – have completed.


I'm pretty sure we missed one

swift-async-queue/Sources/AsyncQueue/FIFOQueue.swift

Line 113 in c4d455f

/// The scheduled task will not execute until all prior tasks have completed.

ahh I missed all the methods added in #7. cdfdfe6

bachand · 2023-02-12T21:24:38Z

Tests/AsyncQueueTests/ActorQueueTests.swift

+        XCTAssertNil(weakCounter)
+    }
+
+    func test_async_retainsAdoptedActorUntilQueueFlushes() async {


This test doesn't flush the queue. Can we rename it to something about how the actor is retained while there is in-progress tasks?

Technically we are flushing the queue by waiting for the async task to complete, but that's not super intuitive! Updated in 1c8cab5

bachand · 2023-02-12T21:47:27Z

Tests/AsyncQueueTests/ActorQueueTests.swift

+        await semaphore.signal()
+    }
+
+    func test_async_taskParameterIsAdoptedActor() async {


bachand · 2023-02-12T21:48:50Z

Tests/AsyncQueueTests/ActorQueueTests.swift

            // Signal the semaphore from the actor queue.
            // If the actor queue were FIFO, this test would hang since this code would never execute:
            // we'd still be waiting for the prior `wait()` tasks to finish.
-            await semaphore.signal()


bachand · 2023-02-12T21:55:01Z

Tests/AsyncQueueTests/ActorQueueTests.swift

-                let count = await counter.count
-                XCTAssertEqual(count, iteration)
+            await systemUnderTest.await { counter in
+                XCTAssertEqual(counter.count, iteration)


So much cleaner.

Co-authored-by: Michael Bachand <[email protected]>

bachand · 2023-02-21T01:04:16Z

README.md

@@ -89,13 +89,16 @@ FIFO execution has a key downside: the queue must wait for all previously enqueu

 Use an `ActorQueue` to send ordered asynchronous tasks to an `actor`’s isolated context from nonisolated or synchronous contexts. Tasks sent to an actor queue are guaranteed to begin executing in the order in which they are enqueued. However, unlike a `FIFOQueue`, execution order is guaranteed only until the first [suspension point](https://docs.swift.org/swift-book/LanguageGuide/Concurrency.html#ID639) within the enqueued task. An `ActorQueue` executes tasks within the its adopted actor’s isolated context, resulting in `ActorQueue` task execution having the same properties as `actor` code execution: code between suspension points is executed atomically, and tasks sent to a single `ActorQueue` can await results from the queue without deadlocking.

-An instance of an `ActorQueue` is designed to be utilized by a single `actor` instance: tasks sent to an `ActorQueue` utilize the isolated context of the queue‘s adopted `actor` to serialize tasks. As such, the lifecycle of any `ActorQueue` should not exceed the lifecycle of its `actor`. It is strongly recommended that an `ActorQueue` be a `let` constant on the adopted `actor`. Additionally, an `actor` utilizing an `ActorQueue` should set the adopted execution context of the queue to `self` within the `actor`’s `init`. Failing to set an adopted execution context prior to enqueuing work on an `ActorQueue` will result in a crash.
+An instance of an `ActorQueue` is designed to be utilized by a single `actor` instance: tasks sent to an `ActorQueue` utilize the isolated context of the queue‘s adopted `actor` to serialize tasks. As such, there are a few requirements that must be met when dealing with an `ActorQueue`:


Maybe "few" -> "couple"?

Since there are two.

😅 was leaving room for me to add more later, but that's certainly a silly approach. Will fix this up in #13

bachand · 2023-02-21T01:04:24Z

README.md

@@ -89,13 +89,16 @@ FIFO execution has a key downside: the queue must wait for all previously enqueu

 Use an `ActorQueue` to send ordered asynchronous tasks to an `actor`’s isolated context from nonisolated or synchronous contexts. Tasks sent to an actor queue are guaranteed to begin executing in the order in which they are enqueued. However, unlike a `FIFOQueue`, execution order is guaranteed only until the first [suspension point](https://docs.swift.org/swift-book/LanguageGuide/Concurrency.html#ID639) within the enqueued task. An `ActorQueue` executes tasks within the its adopted actor’s isolated context, resulting in `ActorQueue` task execution having the same properties as `actor` code execution: code between suspension points is executed atomically, and tasks sent to a single `ActorQueue` can await results from the queue without deadlocking.

-An instance of an `ActorQueue` is designed to be utilized by a single `actor` instance: tasks sent to an `ActorQueue` utilize the isolated context of the queue‘s adopted `actor` to serialize tasks. As such, the lifecycle of any `ActorQueue` should not exceed the lifecycle of its `actor`. It is strongly recommended that an `ActorQueue` be a `let` constant on the adopted `actor`. Additionally, an `actor` utilizing an `ActorQueue` should set the adopted execution context of the queue to `self` within the `actor`’s `init`. Failing to set an adopted execution context prior to enqueuing work on an `ActorQueue` will result in a crash.
+An instance of an `ActorQueue` is designed to be utilized by a single `actor` instance: tasks sent to an `ActorQueue` utilize the isolated context of the queue‘s adopted `actor` to serialize tasks. As such, there are a few requirements that must be met when dealing with an `ActorQueue`:
+1. The lifecycle of any `ActorQueue` should not exceed the lifecycle of its `actor`. It is strongly recommended that an `ActorQueue` be a `let` constant on the adopted `actor`. Enqueuing a task to an `ActorQueue` isntance after its adopted `actor` has been deallocated will result in a crash.


"isntance" -> "instance"

bachand · 2023-02-21T01:05:13Z

README.md


 An `ActorQueue` can easily enqueue tasks that execute on an actor’s isolated context from a nonisolated context in order:
 ```swift
 func testActorQueueOrdering() async {
    actor Counter {
        init() {
+            // Adopting the execution context in `init` satisfies requirement #2 above.


#13) * Make FIFOQueue, ActorQueue, and tests pass strict concurrency checking * Post-merge review feedback from #9 & #12

dfed force-pushed the dfed--isolated-actor-task branch 2 times, most recently from 9e2ed90 to 46fdca4 Compare January 16, 2023 01:03

dfed force-pushed the dfed--isolated-actor-task branch from 46fdca4 to 505f846 Compare January 16, 2023 01:13

dfed commented Jan 16, 2023

View reviewed changes

dfed force-pushed the dfed--isolated-actor-task branch from 505f846 to adbef7a Compare January 16, 2023 01:25

dfed commented Jan 16, 2023

View reviewed changes

dfed requested review from bachand and Gabardone January 16, 2023 01:50

dfed marked this pull request as ready for review January 16, 2023 01:50

dfed force-pushed the dfed--isolated-actor-task branch from 978502c to 5e91ff5 Compare January 16, 2023 07:43

dfed commented Jan 16, 2023

View reviewed changes

dfed force-pushed the dfed--isolated-actor-task branch 4 times, most recently from c67c361 to c343fc7 Compare January 16, 2023 07:56

dfed mentioned this pull request Jan 16, 2023

Rename async and await on both FIFOQueue and ActorQueue #10

Closed

dfed mentioned this pull request Feb 12, 2023

Add consistent examples to README #8

Merged

Base automatically changed from dfed--readme to main February 12, 2023 20:46

dfed added 4 commits February 12, 2023 12:46

Make ActorQueue tasks execute on the target actor's execution context

b44fbb2

Documentation and naming pass

c642170

more docs + tests

da684e4

Add comments to aid a consumer if they hit the unowned crash

a61f857

dfed force-pushed the dfed--isolated-actor-task branch from 5c302b6 to a61f857 Compare February 12, 2023 20:46

bachand approved these changes Feb 12, 2023

View reviewed changes

Feedback from #8

a4871ad

dfed force-pushed the dfed--isolated-actor-task branch from c4d455f to a4871ad Compare February 12, 2023 22:07

dfed and others added 3 commits February 12, 2023 14:46

README wordsmithing

a8797fb

Co-authored-by: Michael Bachand <[email protected]>

Number the requirements

2a63fc0

Warning -> Precondition

ac49f33

dfed added 4 commits February 12, 2023 15:06

DRY

c92e62c

weak

18a0183

Add note to methods added in #7

cdfdfe6

Better test name

1c8cab5

dfed enabled auto-merge (squash) February 12, 2023 23:22

dfed merged commit 8b6a898 into main Feb 12, 2023

dfed deleted the dfed--isolated-actor-task branch February 12, 2023 23:27

bachand reviewed Feb 21, 2023

View reviewed changes

dfed added a commit that referenced this pull request Feb 21, 2023

Post-merge review feedback from #9

c832b94

dfed added a commit that referenced this pull request Feb 21, 2023

Make FIFOQueue, ActorQueue, and tests pass strict concurrency checking (

4fb464f

#13) * Make FIFOQueue, ActorQueue, and tests pass strict concurrency checking * Post-merge review feedback from #9 & #12

		// This works around a bug when compiling with Xcode 14.1: https://github.com/apple/swift/issues/62503
		_ = void

	// Wait all enqueued tasks to finish.
	// Wait for all enqueued tasks to finish.

Uh oh!

Make ActorQueue tasks execute on the target actor's execution context #9

Make ActorQueue tasks execute on the target actor's execution context #9

Uh oh!

Conversation

dfed commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dfed Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfed Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bachand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dfed commented Jan 16, 2023 •

edited

Loading

codecov bot commented Jan 16, 2023 •

edited

Loading

dfed Jan 16, 2023 •

edited

Loading

dfed Jan 16, 2023 •

edited

Loading