Create AsyncQueue #2

dfed · 2022-11-23T05:36:33Z

This PR introduces the AsyncQueue class, which makes it possible to send ordered work to an asynchronous context from a synchronous or non-isolated context.

Any and all API feedback is welcome! Note that the name of the queue changes in #3, which builds upon this work.

dfed · 2022-11-23T05:39:51Z

Sources/AsyncQueue/AsyncQueue.swift

+    // MARK: Private
+
+    private let streamTask: Task<Void, Never>
+    private let taskStreamContinuation: AsyncStream<@Sendable () async -> Void>.Continuation!


Not ideal that we're force-unwrapping here. I originally made this type optional, but figured we really do want to fail LOUDLY if somehow we don't have a taskStreamContinuation and are attempting to append tasks to the queue.

Can we do the failing at the init method so we can initialize this with a non-optional?

We can, but that would force every call-site to force unwrap, which seems pretty gnarly. Since taskStreamContinuation should exist (and it not existing would mean that the world is very broken), I think it's reasonable to crash if it somehow does not exist.

Oh, I meant failing in a very general case. Crashing is fine for something like this where the logic means we would never expect a nil value.

The point was to remove the exclamation mark from here (together with the changes in init proposed).

How's this look? 6e0519c

This is silly, but I'm going to use a force-unwrap and a comment instead of a fatalError. I want 100% coverage 😅

codecov · 2022-11-23T05:41:15Z

Codecov Report

Merging #2 (a458835) into main (10aad91) will increase coverage by 97.50%.
The diff coverage is 97.50%.

❗ Current head a458835 differs from pull request most recent head 2d59b5a. Consider uploading reports for the commit 2d59b5a to get more accurate results

@@            Coverage Diff            @@
##           main       #2       +/-   ##
=========================================
+ Coverage      0   97.50%   +97.50%     
=========================================
  Files         0        1        +1     
  Lines         0       40       +40     
=========================================
+ Hits          0       39       +39     
- Misses        0        1        +1

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

Scripts/build.swift

dfed · 2022-11-23T05:45:33Z

Scripts/build.swift

@@ -174,6 +174,9 @@ for rawPlatform in rawPlatforms {
    if platform.shouldTest {
        xcodeBuildArguments.append("test")
    }
+    xcodeBuildArguments.append("-test-iterations")
+    xcodeBuildArguments.append("100")
+    xcodeBuildArguments.append("-run-tests-until-failure")


This flag stops running tests once they fail. It respects the iterations limit above.

dfed · 2022-11-23T05:52:02Z

Tests/AsyncQueueTests/AsyncQueueTests.swift

+        }
+    }
+
+    func test_await_canReturn() async {


This test and test_await_canThrow below are a little silly but I found them helpful for ensuring that the capability exists as I iterated on the queue's API.

dfed · 2022-11-23T05:53:27Z

Tests/AsyncQueueTests/AsyncQueueTests.swift

@@ -26,8 +26,149 @@ import XCTest

 final class AsyncQueueTests: XCTestCase {


As you read through the below tests: please be thinking of cases I haven't thought of!

dfed · 2022-11-23T06:09:10Z

README.md

+
+A queue that enables ordered sending of events from synchronous to asynchronous code.
+
+## Usage


This is pretty freaking bare-bones but gotta start somewhere.

Sources/AsyncQueue/AsyncQueue.swift

Co-authored-by: Óscar Morales Vivó <38818781+Gabardone@users.noreply.github.com>

Sources/AsyncQueue/AsyncQueue.swift

Co-authored-by: Óscar Morales Vivó <38818781+Gabardone@users.noreply.github.com>

Tests/AsyncQueueTests/AsyncQueueTests.swift

Gabardone

See comments but overall looks like a nice thing to have and hopefully we don't have to complicate it much beyond this (at least until such time as Swift or its libraries adds better support for these use cases).

Sources/AsyncQueue/AsyncQueue.swift

Gabardone

See last comment but it's good to merge either way.

Co-authored-by: Óscar Morales Vivó <38818781+Gabardone@users.noreply.github.com>

Gabardone

I think we ought to merge this already and then keep working on the other PR?

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

I think we ought to merge this already and then keep working on the other PR?

Yup. Was giving more folk a chance to weigh in, but this repo is very beta (first published version will be 0.0.1!) and I'm happy to make breaking API changes for a bit.

Folk who come across this PR: please feel free to leave a post-merge review. I will respond, and will happily open up new PRs to address feedback. Given how early we are in the versioning, post-merge naming feedback is still appreciated!

bachand

Nice!

bachand · 2022-11-28T17:45:13Z

Scripts/build.swift

@@ -159,7 +159,8 @@ for rawPlatform in rawPlatforms {
        "-scheme", "swift-async-queue",
        "-sdk", platform.sdk,
        "-derivedDataPath", platform.derivedDataPath,
-        "-PBXBuildsContinueAfterErrors=0"
+        "-PBXBuildsContinueAfterErrors=0",
+        "OTHER_SWIFT_FLAGS=-warnings-as-errors",


bachand · 2022-11-28T17:46:18Z

README.md

@@ -1,5 +1,31 @@
 # swift-async-queue
-A queue that enables ordered sending of events from synchronous to asynchronous code
+[![Swift Package Manager compatible](https://img.shields.io/badge/SPM-compatible-4BC51D.svg?style=flat)](https://github.com/apple/swift-package-manager)


@dfed we seem to have an issue with some of the later badges. The screenshot is from the main page of the repo.

Yup. I need to publish the pod before those badges will work. I'll remove them for now: 51e98a3

bachand · 2022-11-28T17:48:21Z

Sources/AsyncQueue/AsyncQueue.swift

+        // Continuation will be captured during stream creation, so it is safe to force unwrap here.
+        // If this force-unwrap fails, something is fundamentally broken in the Swift runtime.


bachand · 2022-11-28T17:51:11Z

Sources/AsyncQueue/AsyncQueue.swift

+        // If this force-unwrap fails, something is fundamentally broken in the Swift runtime.
+        taskStreamContinuation = capturedTaskStreamContinuation!
+
+        streamTask = Task.detached(priority: priority) {


The standard Task initializer inherits the configuration from the calling context.

In this case I don't think the calling context has any configuration that we need/want to be inherited. So using Task.detached(…) seems reasonable 👍

bachand · 2022-11-28T17:53:34Z

Sources/AsyncQueue/AsyncQueue.swift

+    // MARK: Public
+
+    /// Schedules an asynchronous task for execution and immediately returns.
+    /// The schedueled task will not execute until all prior tasks have completed.


"schedueled" -> "scheduled"

Same comment for other documentation comments in this file.

Good catch! 2bd88a1

bachand · 2022-11-28T18:20:13Z

Tests/AsyncQueueTests/AsyncQueueTests.swift

+        await systemUnderTest.await { /* Drain the queue */ }
+    }
+
+    func test_async_retainsReceiverUntilFlushed() async {


What is the receiver in this context?

The receiver is the systemUnderTest, i.e. the async queue.

Ah got it. I understand now 👍

bachand · 2022-11-28T18:21:56Z

Tests/AsyncQueueTests/AsyncQueueTests.swift

+        await waitForExpectations(timeout: 1.0)
+    }
+
+    func test_async_doesNotRetainTaskAfterExecution() async {


This was a fun one to write 😄

bachand · 2022-11-28T18:22:54Z

Tests/AsyncQueueTests/AsyncQueueTests.swift

+            guard iteration % 25 == 0 else {
+                // Keep sending async events to the queue.
+                continue
+            }


I don't follow why this code exists in the test. Can you help me understand?

I wanted some way to interleave multiple async events with each await event that was deterministic. We could await after every async, but because the await method requires waiting for the work to finish I felt like the test wouldn't necessarily catch an issue where await wouldn't wait for every enqueued event but would just wait for the first enqueued event...

Short version: this guard could be removed and it'd probably be fine. But I kinda like emitting multiple async events for every await check.

bachand · 2022-11-28T18:24:29Z

Tests/AsyncQueueTests/AsyncQueueTests.swift

+    private actor Counter {
+        func incrementAndExpectCount(equals expectedCount: Int) {
+            increment()
+            XCTAssertEqual(expectedCount, count)


Nit: would be nice to pass through the file and line number so any XCTest failure is easier to debug.

Create AsyncQueue

16e00d3

dfed force-pushed the dfed--create-async-queue branch from 02d3b9d to 16e00d3 Compare November 23, 2022 05:39

dfed commented Nov 23, 2022

View reviewed changes

Run tests 100 times

7cfa79d

dfed commented Nov 23, 2022

View reviewed changes

Scripts/build.swift Show resolved Hide resolved

dfed commented Nov 23, 2022

View reviewed changes

Prevent warnings from sneaking into the repo

4c499a8

dfed mentioned this pull request Nov 23, 2022

Create project structure #1

Merged

dfed commented Nov 23, 2022

View reviewed changes

Start building out the README

f9f4d12

dfed requested a review from Gabardone November 23, 2022 06:06

dfed marked this pull request as ready for review November 23, 2022 06:06

dfed commented Nov 23, 2022

View reviewed changes

Gabardone reviewed Nov 23, 2022

View reviewed changes

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

Gabardone reviewed Nov 23, 2022

View reviewed changes

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

Better comment

fb1c2c2

Co-authored-by: Óscar Morales Vivó <38818781+Gabardone@users.noreply.github.com>

Gabardone reviewed Nov 23, 2022

View reviewed changes

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

Mirror Swift's default parameter usage

67becb0

Co-authored-by: Óscar Morales Vivó <38818781+Gabardone@users.noreply.github.com>

Gabardone reviewed Nov 23, 2022

View reviewed changes

Tests/AsyncQueueTests/AsyncQueueTests.swift Outdated Show resolved Hide resolved

Gabardone reviewed Nov 23, 2022

View reviewed changes

Don't force unwrap – use fatalError with a message instead

6e0519c

Gabardone reviewed Nov 23, 2022

View reviewed changes

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

Gabardone approved these changes Nov 23, 2022

View reviewed changes

dfed and others added 2 commits November 23, 2022 06:56

Update Sources/AsyncQueue/AsyncQueue.swift

157fd83

Co-authored-by: Óscar Morales Vivó <38818781+Gabardone@users.noreply.github.com>

Add test to check for retain cycles

28a6aa1

dfed force-pushed the dfed--create-async-queue branch 3 times, most recently from 07e704a to 0eb5698 Compare November 23, 2022 18:48

dfed force-pushed the dfed--create-async-queue branch from 0eb5698 to c6f0b50 Compare November 23, 2022 18:50

Use a Semaphore to make tests reliable

02cf4e6

dfed force-pushed the dfed--create-async-queue branch from c6f0b50 to 02cf4e6 Compare November 23, 2022 18:50

Ensure tests will not crash if they fail

4f4f7cb

dfed requested a review from bachand November 23, 2022 22:45

dfed added 2 commits November 23, 2022 16:02

Document that the queue is FIFO

af3c0bc

Fix typo

5405eff

dfed mentioned this pull request Nov 24, 2022

Rename AsyncQueue -> FIFOQueue. Create ActorQueue #3

Merged

Wait for async tasks to complete before exiting tests

ec8838a

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

Get to 100% coverage

2d59b5a

Gabardone approved these changes Nov 27, 2022

View reviewed changes

dfed merged commit d2487e2 into main Nov 28, 2022

dfed deleted the dfed--create-async-queue branch November 28, 2022 01:12

bachand reviewed Nov 28, 2022

View reviewed changes

		@@ -26,8 +26,149 @@ import XCTest

		final class AsyncQueueTests: XCTestCase {


		A queue that enables ordered sending of events from synchronous to asynchronous code.

		## Usage

		// Continuation will be captured during stream creation, so it is safe to force unwrap here.
		// If this force-unwrap fails, something is fundamentally broken in the Swift runtime.

Uh oh!

Create AsyncQueue #2

Create AsyncQueue #2

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gabardone Nov 23, 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!

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!

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

Uh oh!

Codecov Report

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Gabardone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Gabardone left a comment

Choose a reason for hiding this comment

Uh oh!

Gabardone left a comment

Choose a reason for hiding this comment

Uh oh!

dfed commented Nov 28, 2022

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!

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 commented Nov 23, 2022 •

edited

Loading

Gabardone Nov 23, 2022 •

edited

Loading

codecov bot commented Nov 23, 2022 •

edited

Loading