Rename AsyncQueue -> FIFOQueue. Create ActorQueue #3

dfed · 2022-11-24T05:47:44Z

I realized that our AsyncQueue is really a FIFOQueue, and that consumers may desire a queue which maintains task execution order when interacting with an actor but allows for reentrancy and re-ordering of tasks due to suspension.

This realization led to the creation of an ActorQueue. This queue is purpose-built to send ordered, asynchronous work to actor instances from a non-isolated context/

Reviewers should feel free to question the type and method names within this PR. I could use another brain here.

dfed · 2022-11-24T05:48:28Z

Tests/AsyncQueueTests/FIFOQueueTests.swift

-
-    // MARK: - Semaphore
-
-    private actor Semaphore {


This Semaphore implementation was moved to its own file without changes beyond its accessibility level.

dfed · 2022-11-24T05:48:44Z

Tests/AsyncQueueTests/FIFOQueueTests.swift

-
-    // MARK: - Counter
-
-    private actor Counter {


This Counter implementation was moved to its own file without changes beyond its accessibility level.

codecov · 2022-11-24T05:52:27Z

Codecov Report

Merging #3 (bf3a0c2) into main (b09b217) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main        #3   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         2    +1     
  Lines           39        95   +56     
=========================================
+ Hits            39        95   +56

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

Sources/AsyncQueue/Semaphore.swift

README.md

Sources/AsyncQueue/ActorQueue.swift

dfed · 2022-11-28T01:13:28Z

Force push was rebasing develop

Sources/AsyncQueue/ActorQueue.swift

…sion in beta semver)

dfed · 2022-12-10T07:07:54Z

Tests/AsyncQueueTests/SemaphoreTests.swift

+
+@testable import AsyncQueue
+
+final class SemaphoreTests: XCTestCase {


These tests are no longer required as part of this PR since Semaphore is no longer living in the Sources directory. I'll remove them shortly and put up a separate PR to add them. They're explicitly a nice-to-have now, though I think it's helpful to have examples of async tests like the below.

dfed · 2022-12-10T07:22:43Z

Apologies for the multiple rounds of revisions here. @bachand @Gabardone this PR is once again ready for another review!

…tch FIFOQueueTests

dfed · 2022-12-10T07:56:35Z

Tests/AsyncQueueTests/ActorQueueTests.swift

+
+@testable import AsyncQueue
+
+final class ActorQueueTests: XCTestCase {


When reviewing this file, it's worth comparing these tests to the ones in FIFOQueueTests

…on context" This reverts commit 139f874.

…n ActorQueueTests to use an ActorQueue

bachand

Nice work @dfed !

bachand · 2023-01-07T19:36:23Z

README.md


-```swift
-let asyncQueue = AsyncQueue()
 ```


Very useful. Thanks for including this.

Thanks for the prompt to do so!

bachand · 2023-01-07T19:38:27Z

README.md

+        /*
+        `async` context that can return a value or throw an error.
+        Executes after all other enqueued work has begun executing.
+        Work enqueued after this task will wait for this task to complete or suspend.


bachand · 2023-01-08T22:09:55Z

Sources/AsyncQueue/ActorQueue.swift

+///
+/// An `ActorQueue` is used to ensure tasks sent from a nonisolated context to a single `actor`'s isolated context begin execution in order.
+/// Here is an example of how an `ActorQueue` should be utilized within an `actor`:
+/// ```


Suggested change

/// ```

/// ```swift

Good catch! bf3a0c2

bachand · 2023-01-08T22:14:15Z

Sources/AsyncQueue/ActorQueue.swift

+    /// The scheduled task will not execute until all prior tasks have completed or suspended.
+    /// - Parameter task: The task to enqueue.
+    /// - Returns: The value returned from the enqueued task.
+    public func await<T>(_ task: @escaping @Sendable () async throws -> T) async throws -> T {


If this function is accepts a throwing task should the above one accept a throwing task too?

Ideally we'd use rethrows here so we only need to declare one method, however I couldn't get rethrows to work here: there's no way today (that I know of) to signal to the compiler that we're only going to throw within withUnsafeThrowingContinuation if and only if the task throws. So I created one method that can throw and one that can't.

bachand · 2023-01-08T22:33:26Z

Tests/AsyncQueueTests/ActorQueueTests.swift

+            // 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.


dfed commented Nov 24, 2022

View reviewed changes

dfed force-pushed the dfed--fifo-vs-actor branch from dd1819e to 0315eca Compare November 24, 2022 05:51

dfed force-pushed the dfed--fifo-vs-actor branch 3 times, most recently from efc1dd3 to 2957e19 Compare November 24, 2022 07:28

dfed marked this pull request as ready for review November 24, 2022 07:32

dfed requested review from bachand and Gabardone November 24, 2022 07:32

dfed commented Nov 24, 2022

View reviewed changes

Sources/AsyncQueue/Semaphore.swift Outdated Show resolved Hide resolved

dfed force-pushed the dfed--fifo-vs-actor branch 2 times, most recently from 54db140 to bd3a337 Compare November 24, 2022 15:06

dfed force-pushed the dfed--create-async-queue branch from a458835 to ec8838a Compare November 24, 2022 15:08

dfed force-pushed the dfed--fifo-vs-actor branch 2 times, most recently from 3be98e5 to dfd4e80 Compare November 24, 2022 15:11

dfed mentioned this pull request Nov 24, 2022

Create AsyncQueue #2

Merged

Gabardone reviewed Nov 27, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

Gabardone reviewed Nov 27, 2022

View reviewed changes

README.md Show resolved Hide resolved

Gabardone reviewed Nov 27, 2022

View reviewed changes

Sources/AsyncQueue/ActorQueue.swift Show resolved Hide resolved

Base automatically changed from dfed--create-async-queue to main November 28, 2022 01:12

dfed added 4 commits November 27, 2022 17:12

Rename AsyncQueue -> FIFOQueue. Create ActorQueue

2b30c2f

Create SemaphoreTests.swift to up coverage

9511ffe

Simplify

0070e39

Get to 100% coverage

67910a7

dfed force-pushed the dfed--fifo-vs-actor branch from dfd4e80 to 67910a7 Compare November 28, 2022 01:13

Enable sending synchronous tasks to the ActorQueue

0565678

dfed commented Nov 28, 2022

View reviewed changes

Sources/AsyncQueue/ActorQueue.swift Outdated Show resolved Hide resolved

Improve explanation of ActorQueue's ordering guarantee

17894cc

dfed added 4 commits December 9, 2022 11:31

Merge branch 'main' into dfed--fifo-vs-actor

e258cb8

Bump version and update README

f70f19d

Simplify ActorExecutor

b8357db

Do not ship Semaphore. Use fewer 'await' calls in ActorExecutor

10ec535

dfed commented Dec 10, 2022

View reviewed changes

Sources/AsyncQueue/ActorQueue.swift Outdated Show resolved Hide resolved

dfed mentioned this pull request Dec 10, 2022

Task created within withUnsafeContinuation breaks actor isolation swiftlang/swift#62503

Open

dfed added 2 commits December 9, 2022 20:07

Add link to Swift bug report

ddff295

Since we're renaming we should bump a major version (i.e. a minor ver…

847c0ed

…sion in beta semver)

dfed commented Dec 10, 2022

View reviewed changes

Delete SemaphoreTests.swift since it is no longer required

01c2dac

dfed mentioned this pull request Dec 10, 2022

Add tests for Semaphore type #5

Merged

dfed added 2 commits December 9, 2022 21:48

Remove unnecessary '@mainactor' decoration on test

47a1753

Add test_async_retainsReceiverUntilFlushed() to ActorQueueTests to ma…

be96303

…tch FIFOQueueTests

dfed commented Dec 10, 2022

View reviewed changes

dfed added 5 commits December 10, 2022 07:33

Simply workaround for Task not executing within actor execution context

139f874

Revert "Simply workaround for Task not executing within actor executi…

2435bfb

…on context" This reverts commit 139f874.

Better test name

3f6d789

Make explicit that queue is deallocated in tests

4b66f40

Update test_async_executesEnqueuedTasksAfterReceiverIsDeallocated() i…

ada3d6c

…n ActorQueueTests to use an ActorQueue

dfed mentioned this pull request Dec 11, 2022

Enable executing FIFO tasks on a desired actor's execution context #6

Merged

Accurate test method naming

771e5e4

dfed mentioned this pull request Dec 11, 2022

Make ActorQueue's tasks isolated to an actor's context #7

Closed

dfed added 2 commits December 11, 2022 09:16

Remove unnecessary await

ea6ade9

Merge branch 'main' into dfed--fifo-vs-actor

1fb3643

bachand approved these changes Jan 8, 2023

View reviewed changes

Add swift tag to multi-line code examples

bf3a0c2

dfed enabled auto-merge (squash) January 8, 2023 23:19

dfed merged commit d08ed02 into main Jan 8, 2023

dfed deleted the dfed--fifo-vs-actor branch January 8, 2023 23:20


		@testable import AsyncQueue

		final class SemaphoreTests: XCTestCase {


		@testable import AsyncQueue

		final class ActorQueueTests: XCTestCase {

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

Uh oh!

Rename AsyncQueue -> FIFOQueue. Create ActorQueue #3

Rename AsyncQueue -> FIFOQueue. Create ActorQueue #3

Uh oh!

Conversation

dfed commented Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfed Nov 24, 2022 • 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!

dfed Nov 24, 2022 • 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!

codecov bot commented Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dfed commented Nov 28, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfed commented Dec 10, 2022

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!

dfed Jan 8, 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!

Uh oh!

dfed commented Nov 24, 2022 •

edited

Loading

dfed Nov 24, 2022 •

edited

Loading

dfed Nov 24, 2022 •

edited

Loading

codecov bot commented Nov 24, 2022 •

edited

Loading

dfed Jan 8, 2023 •

edited

Loading