Fix hang in SingleValueSubject + Timeout #198
Conversation
|
😎 Merged successfully - details. |
📝 WalkthroughWalkthroughAdds a public Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Task/Caller
participant Subject as SingleValueSubject
participant Cont as Continuation
Caller->>Subject: cancel()
alt subject not completed
Subject->>Cont: finish(throwing: CancellationError)
Cont-->>Caller: awaiting call resumes with CancellationError
else subject already completed
Subject-->>Caller: no-op / alreadyCompleted suppressed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas to review closely:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
aim2120
force-pushed
the
aim2120/single-value-subject-timeout-hang
branch
from
1731f46 to
57af766
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Let's do that, that sounds like it would be reasonably expected. |
|
Updated to not throw when |
Tyler-Keith-Thompson
left a comment
Tyler-Keith-Thompson
left a comment
There was a problem hiding this comment.
Thanks for catching this and fixing it! Looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Tests/AfluentTests/WorkerTests/SingleValueSubjectTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/AfluentTests/WorkerTests/SingleValueSubjectTests.swift (1)
Sources/Afluent/Workers/SingleValueSubject.swift (4)
cancel(93-97)send(108-121)send(128-141)send(149-162)
🔇 Additional comments (1)
Tests/AfluentTests/WorkerTests/SingleValueSubjectTests.swift (1)
209-228: Test structure looks good and correctly verifies cooperative cancellation.The test properly verifies the two key behaviors:
- Cancellation causes
CancellationErrorto be thrown- Calling
send()after cancellation doesn't throwThe use of
withMainSerialExecutorandTask.megaYield()appropriately addresses potential race conditions. The comment on line 225 clearly documents the expected behavior.Minor note: The
_ =pattern on line 214 appears to be used to discard the#expectresult. While this works, you may want to verify if this is the idiomatic pattern for the Testing framework or if the result can simply be ignored without explicit assignment.
| @Test func singleValueSubjectCancellation() async throws { | ||
| try await withMainSerialExecutor { | ||
| let subject = SingleValueSubject<Void>() | ||
|
|
||
| let task = Task { | ||
| _ = await #expect(throws: CancellationError.self) { | ||
| try await subject.execute() | ||
| } | ||
| } | ||
|
|
||
| // make sure we don't cancel before the subject operation has begun | ||
| await Task.megaYield() | ||
| task.cancel() | ||
|
|
||
| await task.value | ||
|
|
||
| // should not throw, even though the subject was cancelled | ||
| try subject.send() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider adding test coverage for non-void send variants after cancellation.
The test correctly verifies that send() doesn't throw after cancellation for a SingleValueSubject<Void>. However, based on the relevant code snippets, the same behavior should apply to send(_:) and send(error:). Consider adding similar test cases to ensure comprehensive coverage of the cooperative cancellation behavior.
Example scenarios to test:
- Cancelling a
SingleValueSubject<Int>, then callingsend(42)should not throw - Cancelling a subject, then calling
send(error: someError)should not throw
Suggested test additions:
@Test func singleValueSubjectCancellation_withValue() async throws {
try await withMainSerialExecutor {
let subject = SingleValueSubject<Int>()
let task = Task {
_ = await #expect(throws: CancellationError.self) {
try await subject.execute()
}
}
await Task.megaYield()
task.cancel()
await task.value
// should not throw, even though the subject was cancelled
try subject.send(42)
}
}
@Test func singleValueSubjectCancellation_withError() async throws {
enum TestError: Error { case test }
try await withMainSerialExecutor {
let subject = SingleValueSubject<Int>()
let task = Task {
_ = await #expect(throws: CancellationError.self) {
try await subject.execute()
}
}
await Task.megaYield()
task.cancel()
await task.value
// should not throw, even though the subject was cancelled
try subject.send(error: TestError.test)
}
}🤖 Prompt for AI Agents
In Tests/AfluentTests/WorkerTests/SingleValueSubjectTests.swift around lines
209-228, add coverage for non-void send variants after cancellation: create two
new async tests (e.g., singleValueSubjectCancellation_withValue and
singleValueSubjectCancellation_withError) that mirror the existing Void test
pattern (use withMainSerialExecutor, create SingleValueSubject<Int>(), start a
Task that expects CancellationError from subject.execute(), await
Task.megaYield(), cancel the task, await task.value) and then call try
subject.send(42) in the value test and declare a small TestError enum and call
try subject.send(error: TestError.test) in the error test to ensure these send
variants also do not throw after cancellation.
Bug
Fixes a bug in
SingleValueSubjectthat could cause hangs due to lack of cooperative cancellation.Most notably, it would prevent
timeoutfrom throwing an error if the subject never finished.Before
After
I've verified that without the fix (the cooperative cancellation
cancel()implementation onSingleValueSubject) both new tests will hang.Caveats
With the diff at time of writing, this change is "opt in", due to the fact that a cancelled
SingleValueSubjectwill now throw ifsend()is called and the subject has been cancelled. This is breaking behavioral change that may cause unexpected failures in consumer code (in fact, some of Afluent's own tests would break with this change).Another option to consider would be ignoring the already-finished continuation when
send()is called if theSingleValueSubjectwas finished due to cancellation. This may be a more desirable behavior, since it would enable cooperative cancellation to be rolled out without breaking current consumer expectations.Please let me know what you think and which path forward seems preferable!
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.