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

dfed · 2023-02-17T23:54:14Z

I made this PR by turning on strict concurrency checking, and then squashing the warnings/errors.

dfed · 2023-02-17T23:56:09Z

Package.swift

@@ -22,6 +22,11 @@ let package = Package(
            dependencies: []),


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.

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.

dfed · 2023-02-17T23:58:46Z

Sources/AsyncQueue/ActorQueue.swift

 /// - 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 {


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.

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:

adoptExecutionContext(of:) must be called from init

adoptExecutionContext(of:) must not be called more than once

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.

codecov · 2023-02-18T00:02:25Z

Codecov Report

Merging #13 (0e3149c) into main (5e26f27) will not change coverage.
The diff coverage is 100.00%.

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

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

dfed · 2023-02-18T00:04:30Z

Sources/AsyncQueue/FIFOQueue.swift

@@ -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 {


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.

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.

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?

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.

dfed · 2023-02-18T00:06:40Z

AsyncQueue.podspec

@@ -1,6 +1,6 @@
 Pod::Spec.new do |s|
  s.name     = 'AsyncQueue'
-  s.version  = '0.2.0'
+  s.version  = '0.3.0'


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

dfed · 2023-02-18T00:07:27Z

Tests/AsyncQueueTests/ActorQueueTests.swift

@@ -65,17 +65,17 @@ final class ActorQueueTests: XCTestCase {

    func test_enqueue_taskParameterIsAdoptedActor() async {
        let semaphore = Semaphore()
-        systemUnderTest.enqueue { counter in
-            XCTAssertTrue(counter === self.counter)


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

bachand

One question but overall LGTM. Thanks for addressing feedback from previous PRs.

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

Package.swift

@@ -22,6 +22,11 @@ let package = Package(
            dependencies: []),


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.

bachand · 2023-02-21T01:36:50Z

Sources/AsyncQueue/FIFOQueue.swift

@@ -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 {


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.

bachand · 2023-02-21T01:38:01Z

Sources/AsyncQueue/FIFOQueue.swift

@@ -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 {


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?

dfed commented Feb 17, 2023

View reviewed changes

dfed force-pushed the dfed--complete-concurrency-checking branch from fd7ba52 to eddf9b8 Compare February 18, 2023 00:00

dfed commented Feb 18, 2023

View reviewed changes

Make FIFOQueue, ActorQueue, and tests pass strict concurrency checking

d38ab11

dfed force-pushed the dfed--complete-concurrency-checking branch from eddf9b8 to d38ab11 Compare February 18, 2023 00:05

dfed commented Feb 18, 2023

View reviewed changes

dfed requested review from bachand and Gabardone February 18, 2023 00:07

dfed marked this pull request as ready for review February 18, 2023 00:07

Update README to use new version

0a90b50

dfed mentioned this pull request Feb 21, 2023

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

Merged

dfed added 2 commits February 20, 2023 17:22

Post-merge review feedback from #9

c832b94

Post-merge review feedback from #12

0e3149c

dfed mentioned this pull request Feb 21, 2023

async -> enqueue. await -> enqueueAndWait #12

Merged

bachand approved these changes Feb 21, 2023

View reviewed changes

dfed merged commit 4fb464f into main Feb 21, 2023

dfed deleted the dfed--complete-concurrency-checking branch February 21, 2023 02:23

Uh oh!

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

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

Uh oh!

Conversation

dfed commented Feb 17, 2023

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!

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

Uh oh!

Codecov Report

Uh oh!

dfed Feb 18, 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!

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!

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!

Uh oh!

codecov bot commented Feb 18, 2023 •

edited

Loading

dfed Feb 18, 2023 •

edited

Loading