-
-
Notifications
You must be signed in to change notification settings - Fork 14
Enable executing FIFO tasks on a desired actor's execution context #6
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
Conversation
19e2f43 to
ada5638
Compare
| await systemUnderTest.await { /* Drain the queue */ } | ||
| } | ||
|
|
||
| func test_await_async_areNotReentrant() async { |
There was a problem hiding this comment.
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 Report
@@ Coverage Diff @@
## main #6 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 7 7
Lines 489 703 +214
==========================================
+ Hits 489 703 +214
|
ada5638 to
992261b
Compare
| await systemUnderTest.await { /* Drain the queue */ } | ||
| } | ||
|
|
||
| func test_asyncOn_sendsEventsInOrder() async { |
There was a problem hiding this comment.
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.
992261b to
4b3dde6
Compare
4b3dde6 to
c1aa546
Compare
bachand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @dfed !
Sources/AsyncQueue/FIFOQueue.swift
Outdated
| } | ||
|
|
||
| /// Schedules an asynchronous task for execution and immediately returns. | ||
| /// The scheduled task will not execute until all prior tasks have completed or suspended. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops. Copy paste! 56549e2
Sources/AsyncQueue/FIFOQueue.swift
Outdated
| } | ||
|
|
||
| /// 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sources/AsyncQueue/FIFOQueue.swift
Outdated
| } | ||
|
|
||
| /// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as before.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This PR adds API to the
FIFOQueuethat enablestasks sent to the queue to execute within a particularactor's context.The benefits of this API are:
actor-conforming type can be executed without requiring multiple suspensions.async-decorated properties and methods on a givenactorrequires theawaitkeyword, enabling the casual reader to distinguish which tasks will execute synchronously and which may suspend.