Skip to content

Make FIFOQueue, ActorQueue, and tests pass strict concurrency checking #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 21, 2023
Merged

Make FIFOQueue, ActorQueue, and tests pass strict concurrency checking #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d38ab11
Make FIFOQueue, ActorQueue, and tests pass strict concurrency checking
dfed Feb 17, 2023
0a90b50
Update README to use new version
dfed Feb 18, 2023
c832b94
Post-merge review feedback from #9
dfed Feb 21, 2023
0e3149c
Post-merge review feedback from #12
dfed Feb 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion AsyncQueue.podspec
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Pod::Spec.new do |s|
s.name = 'AsyncQueue'
s.version = '0.2.0'
s.version = '0.3.0'
Copy link
Owner Author

@dfed dfed Feb 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this PR represents a (reasonably small) breaking API change.

s.license = 'MIT'
s.summary = 'A queue that enables ordered sending of events from synchronous to asynchronous code.'
s.homepage = 'https://github.com/dfed/swift-async-queue'
Expand Down
7 changes: 6 additions & 1 deletion Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ let package = Package(
dependencies: []),
Copy link
Owner Author

@dfed dfed Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we'd force the vended target to conform to include strict-concurrency=complete, but doing so would mark this repo as having unsafe build flags. Once we can use enableUpcomingFeature (see below), this will get better.

Copy link
Collaborator

@bachand bachand Feb 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I see that we use unsafe build flags in the test target and that's OK since it's not being consumed by other packages.

.testTarget(
name: "AsyncQueueTests",
dependencies: ["AsyncQueue"]),
dependencies: ["AsyncQueue"],
swiftSettings: [
// TODO: Adopt `enableUpcomingFeature` once available.
// https://github.com/apple/swift-evolution/blob/main/proposals/0362-piecemeal-future-features.md
.unsafeFlags(["-Xfrontend", "-strict-concurrency=complete"])
]),
]
)
8 changes: 4 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ 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, 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.
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 couple 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 `private let` constant on the adopted `actor`. Enqueuing a task to an `ActorQueue` instance after its adopted `actor` has been deallocated will result in a crash.
2. 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 `ActorQueue` can easily enqueue tasks that execute on an actor’s isolated context from a nonisolated context in order:
Expand Down Expand Up @@ -146,7 +146,7 @@ To install swift-async-queue in your iOS project with [Swift Package Manager](ht

```swift
dependencies: [
.package(url: "https://github.com/dfed/swift-async-queue", from: "0.2.0"),
.package(url: "https://github.com/dfed/swift-async-queue", from: "0.3.0"),
]
```

Expand All @@ -156,7 +156,7 @@ To install swift-async-queue in your iOS project with [CocoaPods](http://cocoapo

```
platform :ios, '13.0'
pod 'AsyncQueue', '~> 0.2.0'
pod 'AsyncQueue', '~> 0.3.0'
```

## Contributing
Expand Down
10 changes: 5 additions & 5 deletions Sources/AsyncQueue/ActorQueue.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,9 @@
/// }
/// ```
///
/// - Warning: The `ActorQueue`'s conformance to `@unchecked Sendable` is safe if and only if `adoptExecutionContext(of:)` is called only from the adopted actor's `init` method.
/// - Precondition: The lifecycle of an `ActorQueue` must not exceed that of the adopted actor.
public final class ActorQueue<ActorType: Actor> {
public final class ActorQueue<ActorType: Actor>: @unchecked Sendable {
Copy link
Owner Author

@dfed dfed Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't make ActorQueue be Sendable, then we get the following error in consuming code whenever the ActorQueue instance is accessed from a nonisolated method:

Non-sendable type 'ActorQueue<ActorType>' in asynchronous access to actor-isolated property 'actorQueue' cannot cross actor boundary

This error makes sense: we're accessing a non-sendable let constant outside of the isolated context. I'm looking forward to Swift 6 when I don't need to manually turn on this checking to see these kinds of issues in my packages.

Copy link
Owner Author

@dfed dfed Feb 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also can't make this type Sendable because weakExecutionContext is mutable and therefore the type is not Sendable. But given the API contract of:

  1. adoptExecutionContext(of:) must be called from init
  2. adoptExecutionContext(of:) must not be called more than once
  3. An ActorQueue should be a private let constant on the adopted actor instance

The ActorQueue is effectively Sendable, and therefore we can mark it as such.


// MARK: Initialization

Expand All @@ -78,7 +79,7 @@ public final class ActorQueue<ActorType: Actor> {

// MARK: Public

/// Sets the actor context within which each `enqueue` and `enqueueAndWait` task will execute.
/// Sets the actor context within which each `enqueue` and `enqueueAndWait`ed task will execute.
/// It is recommended that this method be called in the adopted actor’s `init` method.
/// **Must be called prior to enqueuing any work on the receiver.**
///
Expand All @@ -100,7 +101,7 @@ public final class ActorQueue<ActorType: Actor> {
/// The scheduled task will not execute until all prior tasks have completed or suspended.
/// - Parameter task: The task to enqueue. The task's parameter is a reference to the actor whose execution context has been adopted.
/// - Returns: The value returned from the enqueued task.
public func enqueueAndWait<T>(_ task: @escaping @Sendable (isolated ActorType) async -> T) async -> T {
public func enqueueAndWait<T: Sendable>(_ task: @escaping @Sendable (isolated ActorType) async -> T) async -> T {
let executionContext = self.executionContext // Capture/retain the executionContext before suspending.
return await withUnsafeContinuation { continuation in
taskStreamContinuation.yield(ActorTask(executionContext: executionContext) { executionContext in
Expand All @@ -113,7 +114,7 @@ public final class ActorQueue<ActorType: Actor> {
/// The scheduled task will not execute until all prior tasks have completed or suspended.
/// - Parameter task: The task to enqueue. The task's parameter is a reference to the actor whose execution context has been adopted.
/// - Returns: The value returned from the enqueued task.
public func enqueueAndWait<T>(_ task: @escaping @Sendable (isolated ActorType) async throws -> T) async throws -> T {
public func enqueueAndWait<T: Sendable>(_ task: @escaping @Sendable (isolated ActorType) async throws -> T) async throws -> T {
let executionContext = self.executionContext // Capture/retain the executionContext before suspending.
return try await withUnsafeThrowingContinuation { continuation in
taskStreamContinuation.yield(ActorTask(executionContext: executionContext) { executionContext in
Expand Down Expand Up @@ -150,7 +151,6 @@ public final class ActorQueue<ActorType: Actor> {
let executionContext: ActorType
let task: @Sendable (isolated ActorType) async -> Void
}

}

extension Actor {
Expand Down
10 changes: 5 additions & 5 deletions Sources/AsyncQueue/FIFOQueue.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

/// A queue that executes asynchronous tasks enqueued from a nonisolated context in FIFO order.
/// Tasks are guaranteed to begin _and end_ executing in the order in which they are enqueued.
/// Asynchronous tasks sent to this queue work as they would in a `DispatchQueue` type. Attempting to `await` this queue from a task executing on this queue will result in a deadlock.
/// Asynchronous tasks sent to this queue work as they would in a `DispatchQueue` type. Attempting to `enqueueAndWait` this queue from a task executing on this queue will result in a deadlock.
public final class FIFOQueue: Sendable {

// MARK: Initialization
Expand Down Expand Up @@ -71,7 +71,7 @@ public final class FIFOQueue: Sendable {
/// The scheduled task will not execute until all prior tasks – including suspended tasks – have completed.
/// - Parameter task: The task to enqueue.
/// - Returns: The value returned from the enqueued task.
public func enqueueAndWait<T>(_ task: @escaping @Sendable () async -> T) async -> T {
public func enqueueAndWait<T: Sendable>(_ task: @escaping @Sendable () async -> T) async -> T {
await withUnsafeContinuation { continuation in
taskStreamContinuation.yield {
continuation.resume(returning: await task())
Expand All @@ -85,7 +85,7 @@ public final class FIFOQueue: Sendable {
/// - isolatedActor: The actor within which the task is isolated.
/// - task: The task to enqueue.
/// - Returns: The value returned from the enqueued task.
public func enqueueAndWait<ActorType: Actor, T>(on isolatedActor: isolated ActorType, _ task: @escaping @Sendable (isolated ActorType) async -> T) async -> T {
public func enqueueAndWait<ActorType: Actor, T: Sendable>(on isolatedActor: isolated ActorType, _ task: @escaping @Sendable (isolated ActorType) async -> T) async -> T {
Copy link
Owner Author

@dfed dfed Feb 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This T: Sendable conformance was only required on methods that execute on an isolated actor on the FIFOQueue. The warning we saw by not having this conformance was:

Non-sendable type 'T' returned by implicitly asynchronous call to actor-isolated function cannot cross actor boundary

This warning makes sense: T is not Sendable, and therefore how could we be passing it across contexts. However, the fact that this warning only surfaced when we explicitly utilizing an isolated parameter seems like a bug – in all cases we are sending T from an actor's isolated context to the current context. So I made all return types T conform to Sendable, including on the ActorQueue.

Copy link
Collaborator

@bachand bachand Feb 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure I follow, we got a warning in all cases except the ones where the task closure doesn't have an isolated parameter? Making the return type Sendable seems reasonable but I want to try to understand better which methods didn't have a warning so I can try to understand why some methods are warning differently than others.

Copy link
Collaborator

@bachand bachand Feb 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm following correctly I think that these two methods on FIFOQueue didn't have warnings.

public func enqueueAndWait<T: Sendable>(_ task: @escaping @Sendable () async -> T) async -> T
public func enqueueAndWait<T: Sendable>(_ task: @escaping @Sendable () async throws -> T) async throws -> T

Is that right?

Copy link
Owner Author

@dfed dfed Feb 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure I follow, we got a warning in all cases except the ones where the task closure doesn't have an isolated parameter?

Correct!

Making the return type Sendable seems reasonable but I want to try to understand better which methods didn't have a warning so I can try to understand why some methods are warning differently than others.

My best guess is that this is a compiler bug. In all the methods I marked we're crossing a task boundary: from the task that created the async for loop to the call-site. But only in the methods where we have an isolated parameter does the compiler notice that we're crossing a task boundary and therefore only then does it require Sendable conformance.

Is that right?

That is absolutely right.

await withUnsafeContinuation { continuation in
taskStreamContinuation.yield {
continuation.resume(returning: await task(isolatedActor))
Expand All @@ -97,7 +97,7 @@ public final class FIFOQueue: Sendable {
/// The scheduled task will not execute until all prior tasks – including suspended tasks – have completed.
/// - Parameter task: The task to enqueue.
/// - Returns: The value returned from the enqueued task.
public func enqueueAndWait<T>(_ task: @escaping @Sendable () async throws -> T) async throws -> T {
public func enqueueAndWait<T: Sendable>(_ task: @escaping @Sendable () async throws -> T) async throws -> T {
try await withUnsafeThrowingContinuation { continuation in
taskStreamContinuation.yield {
do {
Expand All @@ -115,7 +115,7 @@ public final class FIFOQueue: Sendable {
/// - isolatedActor: The actor within which the task is isolated.
/// - task: The task to enqueue.
/// - Returns: The value returned from the enqueued task.
public func enqueueAndWait<ActorType: Actor, T>(on isolatedActor: isolated ActorType, _ task: @escaping @Sendable (isolated ActorType) async throws -> T) async throws -> T {
public func enqueueAndWait<ActorType: Actor, T: Sendable>(on isolatedActor: isolated ActorType, _ task: @escaping @Sendable (isolated ActorType) async throws -> T) async throws -> T {
try await withUnsafeThrowingContinuation { continuation in
taskStreamContinuation.yield {
do {
Expand Down
8 changes: 4 additions & 4 deletions Tests/AsyncQueueTests/ActorQueueTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,17 @@ final class ActorQueueTests: XCTestCase {

func test_enqueue_taskParameterIsAdoptedActor() async {
let semaphore = Semaphore()
systemUnderTest.enqueue { counter in
XCTAssertTrue(counter === self.counter)
Copy link
Owner Author

@dfed dfed Feb 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

capturing non-sendable self in the sendable closure is not good! So instead we now capture the counter directly.

systemUnderTest.enqueue { [storedCounter = counter] counter in
XCTAssertTrue(counter === storedCounter)
await semaphore.signal()
}

await semaphore.wait()
}

func test_enqueueAndWait_taskParameterIsAdoptedActor() async {
await systemUnderTest.enqueueAndWait { counter in
XCTAssertTrue(counter === self.counter)
await systemUnderTest.enqueueAndWait { [storedCounter = counter] counter in
XCTAssertTrue(counter === storedCounter)
}
}

Expand Down
4 changes: 2 additions & 2 deletions Tests/AsyncQueueTests/FIFOQueueTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ final class FIFOQueueTests: XCTestCase {
systemUnderTest.enqueue {
let isWaiting = await semaphore.isWaiting
// This test will fail occasionally if we aren't executing atomically.
// You can prove this to yourself by replacing `systemUnderTest.async` above with `Task`.
// You can prove this to yourself by replacing `systemUnderTest.enqueue` above with `Task`.
XCTAssertFalse(isWaiting)
// Signal the semaphore before or after we wait – let the scheduler decide.
Task {
Expand All @@ -97,7 +97,7 @@ final class FIFOQueueTests: XCTestCase {
systemUnderTest.enqueue(on: semaphore) { semaphore in
let isWaiting = semaphore.isWaiting
// This test will fail occasionally if we aren't executing atomically.
// You can prove this to yourself by replacing `systemUnderTest.async` above with `Task`.
// You can prove this to yourself by replacing `systemUnderTest.enqueue` above with `Task`.
XCTAssertFalse(isWaiting)
// Signal the semaphore before or after we wait – let the scheduler decide.
Task {
Expand Down