Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 143 additions & 0 deletions Tests/AsyncQueueTests/SemaphoreTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
// MIT License
//
// Copyright (c) 2022 Dan Federman
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in all
// copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.

import XCTest

final class SemaphoreTests: XCTestCase {

// MARK: XCTestCase

override func setUp() async throws {
try await super.setUp()

systemUnderTest = Semaphore()
}

override func tearDown() async throws {
let isWaiting = await systemUnderTest.isWaiting
XCTAssertFalse(isWaiting)

try await super.tearDown()
}

// MARK: Behavior Tests

func test_wait_suspendsUntilEqualNumberOfSignalCalls() async {
/*
This test is tricky to pull off!
Our requirements:
1. We need to call `wait()` before `signal()`
2. We need to ensure that the `wait()` call suspends _before_ we call `signal()`
3. We can't `await` the `wait()` call on the test's queue before calling `signal()` since that would deadlock the test.
4. We must utilize a single actor's isolated context to avoid accidental interleaving when suspending to communicate across actor contexts.

In order to ensure that we are executing the `wait()` calls before we call `signal()` _without awaiting a `wait()` call_,
we utilize the Sempahore's ordered execution context to enqueue ordered `Task`s similar to how an ActorQueue works.
*/

let iterationCount = 1_000
/// A counter that will only be accessed from within the `systemUnderTest`'s context
let unsafeCounter = UnsafeCounter()

for _ in 1...iterationCount {
await systemUnderTest.enqueueAndCount(using: unsafeCounter) { systemUnderTest in
let didSuspend = await systemUnderTest.wait()
XCTAssertTrue(didSuspend)

return { systemUnderTest in
// Signal that the suspended wait call above has resumed.
// This signal allows us to `wait()` for all of these enqueued `wait()` tasks to have completed later in this test.
systemUnderTest.signal()
}
}
}

// Loop one fewer than iterationCount.
for _ in 1..<iterationCount {
await systemUnderTest.execute { systemUnderTest in
systemUnderTest.signal()
}
}

await systemUnderTest.execute { systemUnderTest in
// Give each suspended `wait` task an opportunity to resume (if they were to resume, which they won't) before we check the count.
for _ in 1...iterationCount {
await Task.yield()
}

// The count will still be zero each time because we have executed one more `wait` than `signal` calls.
let completedCountedTasks = unsafeCounter.countedTasksCompleted
XCTAssertEqual(completedCountedTasks, 0)

// Signal one last time, enabling all of the original `wait` calls to resume.
systemUnderTest.signal()

for _ in 1...iterationCount {
// Wait for all enqueued `wait`s to have completed and signaled their completion.
await systemUnderTest.wait()
}

let tasksCompleted = unsafeCounter.countedTasksCompleted
XCTAssertEqual(iterationCount, tasksCompleted)
}
}

func test_wait_doesNotSuspendIfSignalCalledFirst() async {
await systemUnderTest.signal()
let didSuspend = await systemUnderTest.wait()
XCTAssertFalse(didSuspend)
}
Comment on lines +104 to +108
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍


// MARK: Private

private var systemUnderTest = Semaphore()
}

// MARK: - Semaphore Extension

private extension Semaphore {
/// Enqueues an asynchronous task. This method suspends the caller until the asynchronous task has begun, ensuring ordered execution of enqueued tasks.
/// - Parameter task: A unit of work that returns work to execute after the task completes and the count is incremented.
func enqueueAndCount(using counter: UnsafeCounter, _ task: @escaping @Sendable (isolated Semaphore) async -> ((isolated Semaphore) -> Void)?) async {
// Await the start of the soon-to-be-enqueued `Task` with a continuation.
await withCheckedContinuation { continuation in
// Re-enter the actor context but don't wait for the result.
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make sure I follow... the actor context is the semaphore's context? And the reason that this task is on that context is because this method is isolated to the semaphore by the nature of it being an extension on an actor?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious to get your thoughts on this ^ when you have a moment @dfed . Tagging you here in case you missed this one.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I did miss this!

the actor context is the semaphore's context

yes!

the reason that this task is on that context is because this method is isolated to the semaphore by the nature of it being an extension on an actor

yes!

Thoughts on changing "the actor context" to "the semaphore context"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thoughts on changing "the actor context" to "the semaphore context"?

Yeah I think that would increase clarity 👍

Copy link
Owner Author

Choose a reason for hiding this comment

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

Task {
// Now that we're back in the actor context, resume the calling code.
continuation.resume()
let executeAfterIncrement = await task(self)
counter.countedTasksCompleted += 1
executeAfterIncrement?(self)
}
}
}

func execute(_ task: @Sendable (isolated Semaphore) async throws -> Void) async rethrows {
try await task(self)
}
}

// MARK: - UnsafeCounter

private final class UnsafeCounter: @unchecked Sendable {
var countedTasksCompleted = 0
}
Comment on lines +141 to +143
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's OK that this isn't thread-safe since it's only accessed on the actor's context. Is that right?

Copy link
Owner Author

@dfed dfed Feb 8, 2023

Choose a reason for hiding this comment

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

Correct! Hence the /// A counter that will only be accessed from within the systemUnderTest's context comment 🙂

6 changes: 1 addition & 5 deletions codecov.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,5 @@ coverage:
status:
project:
default:
threshold: 0.25%
target: 100%
Copy link
Owner Author

Choose a reason for hiding this comment

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

Similarly, it's a little silly to set a target of 100% coverage. But again, this library is complex, and (thus far) can be written without any runtime assertions that would prove difficult to test.

Given how fundamental this repo will be for consumers, let's start out by holding ourselves to a high standard.

patch: off

ignore:
# This package is not shipped.
- "Tests"
Comment on lines -16 to -18
Copy link
Owner Author

Choose a reason for hiding this comment

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

It's a little silly to require 100% coverage in unit tests but given the complexity of this library I see some benefit.

We can always re-add this ignore if maintaining this level of coverage proves problematic. But I like striving for full coverage in a library like this.