Skip to content

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.

@dfed dfed force-pushed the dfed--isolated-fifo-task branch from 19e2f43 to ada5638 Compare December 11, 2022 18:03
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%> (ø)

@dfed dfed force-pushed the dfed--isolated-fifo-task branch from ada5638 to 992261b Compare December 11, 2022 18:06
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 requested review from bachand and omv-libs December 11, 2022 19:18
@dfed dfed force-pushed the dfed--isolated-fifo-task branch from 992261b to 4b3dde6 Compare December 11, 2022 19:18
@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 force-pushed the dfed--isolated-fifo-task branch from 4b3dde6 to c1aa546 Compare January 8, 2023 23:23
@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 !

}

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

}

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

}

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

3 participants