Skip to content
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.

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

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

Merged
merged 5 commits into from Feb 12, 2023

Conversation

dfed
Copy link
Owner

@dfed dfed commented Dec 11, 2022

This PR adds API to the FIFOQueue that enables tasks sent to the queue to execute within a particular actor's context.

The benefits of this API are:

  1. Multiple interactions with a single actor-conforming type can be executed without requiring multiple suspensions.
  2. 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.

await systemUnderTest.await { /* Drain the queue */ }
}

func test_await_async_areNotReentrant() async {
Copy link
Owner Author

Choose a reason for hiding this comment

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

this test was previously called test_async_isNotReentrant, but technically it was testing that await and async aren't reentrant rather than just testing that async is not reentrant.

@codecov
Copy link

codecov bot commented Dec 11, 2022

Codecov Report

Merging #6 (56549e2) into main (4f38bfe) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              main        #6    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            7         7            
  Lines          489       703   +214     
==========================================
+ Hits           489       703   +214     
Impacted Files Coverage Δ
Sources/AsyncQueue/FIFOQueue.swift 100.00% <100.00%> (ø)
Tests/AsyncQueueTests/FIFOQueueTests.swift 100.00% <100.00%> (ø)

@@ -46,6 +46,32 @@ final class FIFOQueueTests: XCTestCase {
await systemUnderTest.await { /* Drain the queue */ }
}

func test_asyncOn_sendsEventsInOrder() async {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Most of these new tests mirror the tests directly above them that tested the API without the on isolatedActor parameter.

@dfed dfed marked this pull request as ready for review December 11, 2022 19:18
Base automatically changed from dfed--fifo-vs-actor to main January 8, 2023 23:20
@dfed dfed changed the title Enable executing the enqueued task on a desired actor's execution context Enable executing the enqueued FIFO task on a desired actor's execution context Jan 16, 2023
@dfed dfed changed the title Enable executing the enqueued FIFO task on a desired actor's execution context Enable executing FIFO tasks on a desired actor's execution context Jan 16, 2023
Copy link
Collaborator

@bachand bachand left a comment

Choose a reason for hiding this comment

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

Nice work @dfed !

@@ -58,6 +58,15 @@ public final class FIFOQueue: Sendable {
taskStreamContinuation.yield(task)
}

/// Schedules an asynchronous task for execution and immediately returns.
/// The scheduled task will not execute until all prior tasks have completed or suspended.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the FIFO queue so don't we only want this task to execute after all prior tasks have completed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

oops. Copy paste! 56549e2

@@ -70,6 +79,20 @@ public final class FIFOQueue: Sendable {
}
}

/// Schedules an asynchronous task and returns after the task is complete.
/// The scheduled task will not execute until all prior tasks have completed or suspended.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above. My understanding of FIFO queue was that we only wanted a task to execute after the prior tasks completed.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@@ -86,6 +109,24 @@ 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 or suspended.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as before.

Copy link
Owner Author

Choose a reason for hiding this comment

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

let counter = Counter()
for iteration in 1...1_000 {
systemUnderTest.async(on: counter) { counter in
counter.incrementAndExpectCount(equals: iteration)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very nice that you don't need to await.

await systemUnderTest.await { /* Drain the queue */ }
}

func test_async_asyncOn_sendEventsInOrder() async {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

semaphore.signal()
}
// Wait for the concurrent task to complete.
await semaphore.wait()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we still need an await since wait() may suspend. Is that right?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Correct! wait is an async (i.e. possibly suspending) method on the semaphore, whereas signal is not. So we need to await the async method.


await systemUnderTest.await(on: counter) { counter in
let count = counter.count
XCTAssertEqual(count, iteration)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@dfed dfed merged commit a482bb1 into main Feb 12, 2023
@dfed dfed deleted the dfed--isolated-fifo-task branch February 12, 2023 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants