[ECO-4987] Implement lifecycle behaviour that relates to an ongoing operation #85
Conversation
I’ve just moved things around and added some MARK: sections.
This is currently a copy of RoomLifecycle, but will expand to store additional manager state related to each status case.
Missed this in b6c3ddb.
Missed this in 25e5052.
Remove the temporary hasOperationInProgress stored property added in b6c3ddb. It seems to me, at least based on what I’ve implemented so far, that you have an operation in progress if and only if you’re in a certain status case, so I’ve encoded this constraint in the Status type. Might turn out this is false and needs to be revisited later. The operation IDs will be used in an upcoming commit, to allow one operation to wait for the completion of another.
We’ll use this mechanism to implement CHA-RL1d and CHA-RL3c for #52.
Allow tests to find out that one lifecycle operation is waiting for another. This will be needed for testing spec point CHA-RL1d, which states that an ATTACH operation should wait for an in-progress operation to complete before proceeding; I couldn’t think of any way to test this (e.g. looking for some side effect) with the information that we currently have, hence this testing API.
Will use this in some upcoming tests. Intended for use with the API added in 57e1459.
(From the same spec as referenced in 25e5052.)
WalkthroughThe changes in this pull request primarily focus on the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
Sources/AblyChat/RoomLifecycleManager.swift (4)
52-52: Consider making thestatuspropertyprivate(set)if external read access is neededThe
statusproperty is declared as a mutable private variable. If it needs to be read externally but only modified within theRoomLifecycleManager, consider changing it toprivate(set)to allow external read access while preventing external modifications.Apply this change:
-private var status: Status +private(set) var status: Status
379-380: Clarify the definition ofhasOperationInProgressThe comment above
hasOperationInProgressmentions uncertainty about its exact meaning. For maintainability, consider clarifying when an operation is considered in progress and when it is completed. This will help other developers understand the flow and prevent potential misuse of the property.
478-483: Handle specific errors instead of catching all errorsIn the
performAnOperationmethod, you're catching all errors but rethrowing them immediately. Consider catching specific error types or removing the do-catch block if it's not adding value. This can make error handling clearer and prevent unintended swallowing of errors.
718-719: Redundant comment in RELEASE operationThe comment
// CHA-RL3cis repeated above and below thechangeStatuscall. Consider removing the redundant comment to improve code readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- Sources/AblyChat/RoomLifecycleManager.swift (13 hunks)
- Tests/AblyChatTests/RoomLifecycleManagerTests.swift (12 hunks)
🧰 Additional context used
📓 Learnings (3)
📓 Common learnings
Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Sources/AblyChat/RoomLifecycleManager.swift:0-0 Timestamp: 2024-09-18T18:34:37.252Z Learning: In `RoomLifecycleManager`, handling of unexpected contributor states after attach failure will be addressed in issue #49.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Sources/AblyChat/RoomLifecycleManager.swift:0-0 Timestamp: 2024-10-08T15:58:47.376Z Learning: In `RoomLifecycleManager`, handling of unexpected contributor states after attach failure will be addressed in issue #49.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631 Timestamp: 2024-10-01T12:55:21.968Z Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631 Timestamp: 2024-10-08T15:58:47.376Z Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.Sources/AblyChat/RoomLifecycleManager.swift (4)
Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Sources/AblyChat/RoomLifecycleManager.swift:0-0 Timestamp: 2024-09-18T18:34:37.252Z Learning: In `RoomLifecycleManager`, handling of unexpected contributor states after attach failure will be addressed in issue #49.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Sources/AblyChat/RoomLifecycleManager.swift:0-0 Timestamp: 2024-10-08T15:58:47.376Z Learning: In `RoomLifecycleManager`, handling of unexpected contributor states after attach failure will be addressed in issue #49.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Sources/AblyChat/RoomLifecycleManager.swift:217-220 Timestamp: 2024-10-01T19:44:34.032Z Learning: In `RoomLifecycleManager`, handling of missing `stateChange.reason` when `stateChange.resumed` is `false` is being tracked in issue #74. Future reviews should be aware of this planned work.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Sources/AblyChat/RoomLifecycleManager.swift:217-220 Timestamp: 2024-10-08T15:58:47.376Z Learning: In `RoomLifecycleManager`, handling of missing `stateChange.reason` when `stateChange.resumed` is `false` is being tracked in issue #74. Future reviews should be aware of this planned work.Tests/AblyChatTests/RoomLifecycleManagerTests.swift (10)
Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631 Timestamp: 2024-10-08T15:58:47.376Z Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631 Timestamp: 2024-10-01T12:55:21.968Z Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:335-335 Timestamp: 2024-09-23T19:56:39.435Z Learning: In Swift Testing, test method names do not need to start with `test`.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:499-504 Timestamp: 2024-10-01T12:47:28.179Z Learning: In test closures, parameters like `callCount` may be used to track invocation counts correctly across invocations.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:499-504 Timestamp: 2024-10-08T15:58:47.376Z Learning: In test closures, parameters like `callCount` may be used to track invocation counts correctly across invocations.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:335-335 Timestamp: 2024-10-08T15:58:47.376Z Learning: In Swift Testing, test method names do not need to start with `test`.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:941-941 Timestamp: 2024-10-01T19:46:16.467Z Learning: When a TODO comment references an existing GitHub issue, it's acceptable to leave it in the code without requesting its removal.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:941-941 Timestamp: 2024-10-08T15:58:47.376Z Learning: When a TODO comment references an existing GitHub issue, it's acceptable to leave it in the code without requesting its removal.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:836-836 Timestamp: 2024-10-01T19:46:04.461Z Learning: Before suggesting the removal of TODO comments, check if they reference existing GitHub issues that track the pending work.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:836-836 Timestamp: 2024-10-08T15:58:47.376Z Learning: Before suggesting the removal of TODO comments, check if they reference existing GitHub issues that track the pending work.
🔇 Additional comments (15)
Sources/AblyChat/RoomLifecycleManager.swift (3)
246-253: Potential concurrency issue inchangeStatusmethodIn the
changeStatus(to:)method, thestatusis updated, and thenemitStatusChangeis called to notify subscribers. SinceRoomLifecycleManageris an actor, these operations are isolated. However, ensure that any asynchronous calls withinemitStatusChangedo not introduce concurrency issues or race conditions.
435-438: Verify thread safety when accessingoperationResultContinuationsIn
waitForCompletionOfOperationWithID, you modifyoperationResultContinuationsby adding continuations. Since this structure is mutable, ensure that all accesses to it are properly isolated within the actor to prevent race conditions.
Line range hint
563-567: Confirm the timing ofemitPendingDiscontinuityEventsIn
bodyOfAttachOperation, the call toemitPendingDiscontinuityEventsis preceded by a TODO comment about whether it's part of the ATTACH operation. Clarify the sequence according to the specification to ensure correct behavior.Tests/AblyChatTests/RoomLifecycleManagerTests.swift (12)
31-31: Update parameter to acceptStatusinstead ofRoomLifecycle.Changing the parameter
forTestingWhatHappensWhenCurrentlyInto acceptRoomLifecycleManager<MockRoomLifecycleContributor>.Status?provides a more precise representation of the manager's state in tests. This enhances the clarity and specificity of the test setups.
100-102: Consistent use of updatedStatusin test initialization.The test
attach_whenReleasingnow correctly initializes the manager with the.releasingstatus using the updated parameter. This ensures that the test accurately reflects the releasing state.
128-174: Well-structured test for operation sequencing with concurrent operations.The new test
attach_ifOtherOperationInProgress_waitsForItToCompleteeffectively verifies that anATTACHoperation waits for a pending operation to complete, as specified in CHA-RL1d. The use of mock operations and async tasks is appropriate and clearly demonstrates the intended behavior.
443-445: Proper initialization of manager state indetach_whenReleasing.Updating the manager initialization to use the
.releasingstatus ensures that thedetach_whenReleasingtest accurately reflects the intended state when testing error handling.
653-697: Accurate test for handling concurrentRELEASEoperations.The test
release_whenReleasingcorrectly verifies that a secondRELEASEoperation waits for the first to complete, adhering to CHA-RL3c. The implementation effectively checks that the second release does not duplicate work and that operations are properly sequenced.
850-853: Recording pending discontinuity events during ongoing operations.In
contributorUpdate_withResumedFalse_withOperationInProgress_recordsPendingDiscontinuityEvent, the manager correctly records a pending discontinuity event when an update occurs during an operation. This aligns with CHA-RL4a3 and ensures that events are not lost.
881-884: Immediate discontinuity handling without ongoing operations.The test
contributorUpdate_withResumedTrue_withNoOperationInProgress_emitsDiscontinuityEventproperly checks that discontinuity events are emitted immediately when no operation is in progress, fulfilling CHA-RL4a4.
912-915: Correct handling ofATTACHEDevents withresumedfalse during operations.The test
contributorAttachEvent_withResumeFalse_withOperationInProgress_recordsPendingDiscontinuityEventensures that the manager records discontinuity events appropriately, adhering to CHA-RL4b1.
947-950: Handling contributor failure events without operations in progress.In
contributorFailedEvent_withNoOperationInProgress, the manager transitions to a failed state upon receiving a contributor'sFAILEDevent, which is consistent with the specification. Detaching all contributors in response is appropriate.
982-982: Ensuring room transitions toATTACHEDwhen all contributors are attached.The test
contributorAttachedEvent_withNoOperationInProgress_roomNotAttached_allContributorsAttachedcorrectly verifies that the room transitions toATTACHEDwhen all contributors are in theATTACHEDstate, as specified in CHA-RL4b8.
Line range hint
1015-1041: Validating that room does not transition if not all contributors are attached.The test
contributorAttachedEvent_withNoOperationInProgress_roomNotAttached_notAllContributorsAttachedappropriately checks that the room does not transition toATTACHEDif any contributor is not attached.
1049-1052: Proper handling ofSUSPENDEDevents without ongoing operations.The test
contributorSuspendedEvent_withNoOperationInProgressconfirms that the manager transitions the room toSUSPENDEDwhen a contributor enters theSUSPENDEDstate, which aligns with CHA-RL4b9.
lawrence-forooghian
deleted the
52-implement-awareness-of-other-lifecycle-ops
branch
Does a bunch of groundwork, and then implements spec points CHA-RL1d and CHA-RL3c. See commit mesages for more details.
Resolves #52.
Summary by CodeRabbit
New Features
Bug Fixes
Tests