From 7cdd467566924058f426ce53698a631520be04cb Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Mon, 14 Oct 2024 17:29:46 -0300 Subject: [PATCH 01/11] Organise RoomLifecycleManager.swift file a bit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I’ve just moved things around and added some MARK: sections. --- Sources/AblyChat/RoomLifecycleManager.swift | 139 +++++++++++--------- 1 file changed, 77 insertions(+), 62 deletions(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index dbbf34ee..1135b40b 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -41,55 +41,24 @@ internal protocol RoomLifecycleContributor: Identifiable, Sendable { } internal actor RoomLifecycleManager { - /// Stores manager state relating to a given contributor. - private struct ContributorAnnotation { - // TODO: Not clear whether there can be multiple or just one (asked in https://github.com/ably/specification/pull/200/files#r1781927850) - var pendingDiscontinuityEvents: [ARTErrorInfo] = [] - } + // MARK: - Constant properties - internal private(set) var current: RoomLifecycle // TODO: This currently allows the the tests to inject a value in order to test the spec points that are predicated on whether “a channel lifecycle operation is in progress”. In https://github.com/ably-labs/ably-chat-swift/issues/52 we’ll set this property based on whether there actually is a lifecycle operation in progress. private let hasOperationInProgress: Bool - /// Manager state that relates to individual contributors, keyed by contributors’ ``Contributor.id``. Stored separately from ``contributors`` so that the latter can be a `let`, to make it clear that the contributors remain fixed for the lifetime of the manager. - private var contributorAnnotations: ContributorAnnotations - - /// Provides a `Dictionary`-like interface for storing manager state about individual contributors. - private struct ContributorAnnotations { - private var storage: [Contributor.ID: ContributorAnnotation] - - init(contributors: [Contributor], pendingDiscontinuityEvents: [Contributor.ID: [ARTErrorInfo]]) { - storage = contributors.reduce(into: [:]) { result, contributor in - result[contributor.id] = .init(pendingDiscontinuityEvents: pendingDiscontinuityEvents[contributor.id] ?? []) - } - } - - /// It is a programmer error to call this subscript getter with a contributor that was not one of those passed to ``init(contributors:pendingDiscontinuityEvents)``. - subscript(_ contributor: Contributor) -> ContributorAnnotation { - get { - guard let annotation = storage[contributor.id] else { - preconditionFailure("Expected annotation for \(contributor)") - } - return annotation - } - - set { - storage[contributor.id] = newValue - } - } - - mutating func clearPendingDiscontinuityEvents() { - storage = storage.mapValues { annotation in - var newAnnotation = annotation - newAnnotation.pendingDiscontinuityEvents = [] - return newAnnotation - } - } - } - private let logger: InternalLogger private let clock: SimpleClock private let contributors: [Contributor] + + // MARK: - Variable properties + + internal private(set) var current: RoomLifecycle + /// Manager state that relates to individual contributors, keyed by contributors’ ``Contributor.id``. Stored separately from ``contributors`` so that the latter can be a `let`, to make it clear that the contributors remain fixed for the lifetime of the manager. + private var contributorAnnotations: ContributorAnnotations private var listenForStateChangesTask: Task! + // TODO: clean up old subscriptions (https://github.com/ably-labs/ably-chat-swift/issues/36) + private var subscriptions: [Subscription] = [] + + // MARK: - Initializers and `deinit` internal init( contributors: [Contributor], @@ -172,14 +141,48 @@ internal actor RoomLifecycleManager { listenForStateChangesTask.cancel() } - #if DEBUG - internal func testsOnly_pendingDiscontinuityEvents(for contributor: Contributor) -> [ARTErrorInfo] { - contributorAnnotations[contributor].pendingDiscontinuityEvents + // MARK: - Types for contributor annotations + + /// Stores manager state relating to a given contributor. + private struct ContributorAnnotation { + // TODO: Not clear whether there can be multiple or just one (asked in https://github.com/ably/specification/pull/200/files#r1781927850) + var pendingDiscontinuityEvents: [ARTErrorInfo] = [] + } + + /// Provides a `Dictionary`-like interface for storing manager state about individual contributors. + private struct ContributorAnnotations { + private var storage: [Contributor.ID: ContributorAnnotation] + + init(contributors: [Contributor], pendingDiscontinuityEvents: [Contributor.ID: [ARTErrorInfo]]) { + storage = contributors.reduce(into: [:]) { result, contributor in + result[contributor.id] = .init(pendingDiscontinuityEvents: pendingDiscontinuityEvents[contributor.id] ?? []) + } } - #endif - // TODO: clean up old subscriptions (https://github.com/ably-labs/ably-chat-swift/issues/36) - private var subscriptions: [Subscription] = [] + /// It is a programmer error to call this subscript getter with a contributor that was not one of those passed to ``init(contributors:pendingDiscontinuityEvents)``. + subscript(_ contributor: Contributor) -> ContributorAnnotation { + get { + guard let annotation = storage[contributor.id] else { + preconditionFailure("Expected annotation for \(contributor)") + } + return annotation + } + + set { + storage[contributor.id] = newValue + } + } + + mutating func clearPendingDiscontinuityEvents() { + storage = storage.mapValues { annotation in + var newAnnotation = annotation + newAnnotation.pendingDiscontinuityEvents = [] + return newAnnotation + } + } + } + + // MARK: - Room status changes internal func onChange(bufferingPolicy: BufferingPolicy) -> Subscription { let subscription: Subscription = .init(bufferingPolicy: bufferingPolicy) @@ -187,6 +190,23 @@ internal actor RoomLifecycleManager { return subscription } + /// Updates ``current`` and emits a status change event. + private func changeStatus(to new: RoomLifecycle) { + logger.log(message: "Transitioning from \(current) to \(new)", level: .info) + let previous = current + current = new + let statusChange = RoomStatusChange(current: current, previous: previous) + emitStatusChange(statusChange) + } + + private func emitStatusChange(_ change: RoomStatusChange) { + for subscription in subscriptions { + subscription.emit(change) + } + } + + // MARK: - Handling contributor state changes + #if DEBUG // TODO: clean up old subscriptions (https://github.com/ably-labs/ably-chat-swift/issues/36) /// Supports the ``testsOnly_subscribeToHandledContributorStateChanges()`` method. @@ -204,6 +224,10 @@ internal actor RoomLifecycleManager { stateChangeHandledSubscriptions.append(subscription) return subscription } + + internal func testsOnly_pendingDiscontinuityEvents(for contributor: Contributor) -> [ARTErrorInfo] { + contributorAnnotations[contributor].pendingDiscontinuityEvents + } #endif /// Implements CHA-RL4b’s contributor state change handling. @@ -294,20 +318,7 @@ internal actor RoomLifecycleManager { #endif } - /// Updates ``current`` and emits a status change event. - private func changeStatus(to new: RoomLifecycle) { - logger.log(message: "Transitioning from \(current) to \(new)", level: .info) - let previous = current - current = new - let statusChange = RoomStatusChange(current: current, previous: previous) - emitStatusChange(statusChange) - } - - private func emitStatusChange(_ change: RoomStatusChange) { - for subscription in subscriptions { - subscription.emit(change) - } - } + // MARK: - ATTACH operation /// Implements CHA-RL1’s `ATTACH` operation. internal func performAttachOperation() async throws { @@ -400,6 +411,8 @@ internal actor RoomLifecycleManager { } } + // MARK: - DETACH operation + /// Implements CHA-RL2’s DETACH operation. internal func performDetachOperation() async throws { switch current { @@ -479,6 +492,8 @@ internal actor RoomLifecycleManager { changeStatus(to: .detached) } + // MARK: - RELEASE operation + /// Implementes CHA-RL3’s RELEASE operation. internal func performReleaseOperation() async { switch current { From bdd9b648c0464ce80088a04ea9ba3f5e6b2d19ac Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 10 Oct 2024 09:59:02 -0300 Subject: [PATCH 02/11] Give RoomLifecycleManager an internal status type This is currently a copy of RoomLifecycle, but will expand to store additional manager state related to each status case. --- Sources/AblyChat/RoomLifecycleManager.swift | 77 ++++++++++++++----- .../RoomLifecycleManagerTests.swift | 10 +-- 2 files changed, 64 insertions(+), 23 deletions(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index 1135b40b..ac0b963f 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -51,7 +51,7 @@ internal actor RoomLifecycleManager { // MARK: - Variable properties - internal private(set) var current: RoomLifecycle + private var status: Status /// Manager state that relates to individual contributors, keyed by contributors’ ``Contributor.id``. Stored separately from ``contributors`` so that the latter can be a `let`, to make it clear that the contributors remain fixed for the lifetime of the manager. private var contributorAnnotations: ContributorAnnotations private var listenForStateChangesTask: Task! @@ -66,7 +66,7 @@ internal actor RoomLifecycleManager { clock: SimpleClock ) async { await self.init( - current: nil, + status: nil, hasOperationInProgress: nil, pendingDiscontinuityEvents: [:], contributors: contributors, @@ -77,7 +77,7 @@ internal actor RoomLifecycleManager { #if DEBUG internal init( - testsOnly_current current: RoomLifecycle? = nil, + testsOnly_status status: Status? = nil, testsOnly_hasOperationInProgress hasOperationInProgress: Bool? = nil, testsOnly_pendingDiscontinuityEvents pendingDiscontinuityEvents: [Contributor.ID: [ARTErrorInfo]]? = nil, contributors: [Contributor], @@ -85,7 +85,7 @@ internal actor RoomLifecycleManager { clock: SimpleClock ) async { await self.init( - current: current, + status: status, hasOperationInProgress: hasOperationInProgress, pendingDiscontinuityEvents: pendingDiscontinuityEvents, contributors: contributors, @@ -96,14 +96,14 @@ internal actor RoomLifecycleManager { #endif private init( - current: RoomLifecycle?, + status: Status?, hasOperationInProgress: Bool?, pendingDiscontinuityEvents: [Contributor.ID: [ARTErrorInfo]]?, contributors: [Contributor], logger: InternalLogger, clock: SimpleClock ) async { - self.current = current ?? .initialized + self.status = status ?? .initialized self.hasOperationInProgress = hasOperationInProgress ?? false self.contributors = contributors contributorAnnotations = .init(contributors: contributors, pendingDiscontinuityEvents: pendingDiscontinuityEvents ?? [:]) @@ -141,6 +141,43 @@ internal actor RoomLifecycleManager { listenForStateChangesTask.cancel() } + // MARK: - Type for room status + + internal enum Status: Equatable { + case initialized + case attaching + case attached + case detaching + case detached + case suspended(error: ARTErrorInfo) + case failed(error: ARTErrorInfo) + case releasing + case released + + internal var toRoomLifecycle: RoomLifecycle { + switch self { + case .initialized: + .initialized + case .attaching: + .attaching + case .attached: + .attached + case .detaching: + .detaching + case .detached: + .detached + case let .suspended(error): + .suspended(error: error) + case let .failed(error): + .failed(error: error) + case .releasing: + .releasing + case .released: + .released + } + } + } + // MARK: - Types for contributor annotations /// Stores manager state relating to a given contributor. @@ -182,7 +219,11 @@ internal actor RoomLifecycleManager { } } - // MARK: - Room status changes + // MARK: - Room status and its changes + + internal var current: RoomLifecycle { + status.toRoomLifecycle + } internal func onChange(bufferingPolicy: BufferingPolicy) -> Subscription { let subscription: Subscription = .init(bufferingPolicy: bufferingPolicy) @@ -190,12 +231,12 @@ internal actor RoomLifecycleManager { return subscription } - /// Updates ``current`` and emits a status change event. - private func changeStatus(to new: RoomLifecycle) { - logger.log(message: "Transitioning from \(current) to \(new)", level: .info) - let previous = current - current = new - let statusChange = RoomStatusChange(current: current, previous: previous) + /// Updates ``status`` and emits a status change event. + private func changeStatus(to new: Status) { + logger.log(message: "Transitioning from \(status) to \(new)", level: .info) + let previous = status + status = new + let statusChange = RoomStatusChange(current: status.toRoomLifecycle, previous: previous.toRoomLifecycle) emitStatusChange(statusChange) } @@ -271,7 +312,7 @@ internal actor RoomLifecycleManager { contributorAnnotations[contributor].pendingDiscontinuityEvents.append(reason) } - } else if current != .attached { + } else if status != .attached { if await (contributors.async.map { await $0.channel.state }.allSatisfy { $0 == .attached }) { // CHA-RL4b8 logger.log(message: "Now that all contributors are ATTACHED, transitioning room to ATTACHED", level: .info) @@ -322,7 +363,7 @@ internal actor RoomLifecycleManager { /// Implements CHA-RL1’s `ATTACH` operation. internal func performAttachOperation() async throws { - switch current { + switch status { case .attached: // CHA-RL1a return @@ -415,7 +456,7 @@ internal actor RoomLifecycleManager { /// Implements CHA-RL2’s DETACH operation. internal func performDetachOperation() async throws { - switch current { + switch status { case .detached: // CHA-RL2a return @@ -461,7 +502,7 @@ internal actor RoomLifecycleManager { } // This check is CHA-RL2h2 - if !current.isFailed { + if !status.toRoomLifecycle.isFailed { changeStatus(to: .failed(error: error)) } default: @@ -496,7 +537,7 @@ internal actor RoomLifecycleManager { /// Implementes CHA-RL3’s RELEASE operation. internal func performReleaseOperation() async { - switch current { + switch status { case .released: // CHA-RL3a return diff --git a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift index 2cb0558b..e952e190 100644 --- a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift +++ b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift @@ -28,14 +28,14 @@ struct RoomLifecycleManagerTests { } private func createManager( - forTestingWhatHappensWhenCurrentlyIn current: RoomLifecycle? = nil, + forTestingWhatHappensWhenCurrentlyIn status: RoomLifecycleManager.Status? = nil, forTestingWhatHappensWhenHasOperationInProgress hasOperationInProgress: Bool? = nil, forTestingWhatHappensWhenHasPendingDiscontinuityEvents pendingDiscontinuityEvents: [MockRoomLifecycleContributor.ID: [ARTErrorInfo]]? = nil, contributors: [MockRoomLifecycleContributor] = [], clock: SimpleClock = MockSimpleClock() ) async -> RoomLifecycleManager { await .init( - testsOnly_current: current, + testsOnly_status: status, testsOnly_hasOperationInProgress: hasOperationInProgress, testsOnly_pendingDiscontinuityEvents: pendingDiscontinuityEvents, contributors: contributors, @@ -911,9 +911,9 @@ struct RoomLifecycleManagerTests { createContributor(initialState: .detached), ] - let initialManagerState = RoomLifecycle.initialized // arbitrary non-ATTACHED + let initialManagerStatus = RoomLifecycleManager.Status.initialized // arbitrary non-ATTACHED let manager = await createManager( - forTestingWhatHappensWhenCurrentlyIn: initialManagerState, + forTestingWhatHappensWhenCurrentlyIn: initialManagerStatus, forTestingWhatHappensWhenHasOperationInProgress: false, contributors: contributors ) @@ -932,7 +932,7 @@ struct RoomLifecycleManagerTests { } // Then: The room status does not change - #expect(await manager.current == initialManagerState) + #expect(await manager.current == initialManagerStatus.toRoomLifecycle) } // @specPartial CHA-RL4b9 - Haven’t implemented the part that refers to "transient disconnect timeouts"; TODO do this (https://github.com/ably-labs/ably-chat-swift/issues/48). Nor have I implemented "the room enters the RETRY loop"; TODO do this (https://github.com/ably-labs/ably-chat-swift/issues/51) From 43559ba2f31bb4f914efc6124d16155f03fa313d Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 10 Oct 2024 11:55:51 -0300 Subject: [PATCH 03/11] Add missing condition to test descriptions Missed this in b6c3ddb. --- Tests/AblyChatTests/RoomLifecycleManagerTests.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift index e952e190..579f2a64 100644 --- a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift +++ b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift @@ -871,7 +871,7 @@ struct RoomLifecycleManagerTests { // @specOneOf(1/2) CHA-RL4b8 @Test func contributorAttachedEvent_withNoOperationInProgress_roomNotAttached_allContributorsAttached() async throws { - // Given: A RoomLifecycleManager, not in the ATTACHED state, all of whose contributors are in the ATTACHED state (to satisfy the condition of CHA-RL4b8; for the purposes of this test I don’t care that they’re in this state even _before_ the state change of the When) + // Given: A RoomLifecycleManager, with no operation in progress and not in the ATTACHED state, all of whose contributors are in the ATTACHED state (to satisfy the condition of CHA-RL4b8; for the purposes of this test I don’t care that they’re in this state even _before_ the state change of the When) let contributors = [ createContributor(initialState: .attached), createContributor(initialState: .attached), @@ -905,7 +905,7 @@ struct RoomLifecycleManagerTests { // @specOneOf(2/2) CHA-RL4b8 - Tests that the specified side effect doesn’t happen if part of the condition (i.e. all contributors now being ATTACHED) is not met @Test func contributorAttachedEvent_withNoOperationInProgress_roomNotAttached_notAllContributorsAttached() async throws { - // Given: A RoomLifecycleManager, not in the ATTACHED state, one of whose contributors is not in the ATTACHED state state (to simulate the condition of CHA-RL4b8 not being met; for the purposes of this test I don’t care that they’re in this state even _before_ the state change of the When) + // Given: A RoomLifecycleManager, with no operation in progress and not in the ATTACHED state, one of whose contributors is not in the ATTACHED state state (to simulate the condition of CHA-RL4b8 not being met; for the purposes of this test I don’t care that they’re in this state even _before_ the state change of the When) let contributors = [ createContributor(initialState: .attached), createContributor(initialState: .detached), From 7845f1f1a78c7994b6295af3d701c75337754d80 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 10 Oct 2024 13:55:13 -0300 Subject: [PATCH 04/11] Add spec point comment Missed this in 25e5052. --- Sources/AblyChat/RoomLifecycleManager.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index ac0b963f..dbac25ab 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -549,6 +549,7 @@ internal actor RoomLifecycleManager { break } + // CHA-RL3c changeStatus(to: .releasing) // CHA-RL3d From cc53b282ad397b272113fe98b006bf0f4e3d3694 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 10 Oct 2024 13:57:56 -0300 Subject: [PATCH 05/11] Fix typo --- Sources/AblyChat/RoomLifecycleManager.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index dbac25ab..2dac8cb2 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -535,7 +535,7 @@ internal actor RoomLifecycleManager { // MARK: - RELEASE operation - /// Implementes CHA-RL3’s RELEASE operation. + /// Implements CHA-RL3’s RELEASE operation. internal func performReleaseOperation() async { switch status { case .released: From 17152786df10283a6b7c1e520c830e2361a8d69b Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 10 Oct 2024 10:24:51 -0300 Subject: [PATCH 06/11] Track whether a room lifecycle operation is in progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- Sources/AblyChat/RoomLifecycleManager.swift | 53 ++++++++++++++----- .../RoomLifecycleManagerTests.swift | 39 +++++++++----- 2 files changed, 67 insertions(+), 25 deletions(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index 2dac8cb2..f5fd3da2 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -43,8 +43,6 @@ internal protocol RoomLifecycleContributor: Identifiable, Sendable { internal actor RoomLifecycleManager { // MARK: - Constant properties - // TODO: This currently allows the the tests to inject a value in order to test the spec points that are predicated on whether “a channel lifecycle operation is in progress”. In https://github.com/ably-labs/ably-chat-swift/issues/52 we’ll set this property based on whether there actually is a lifecycle operation in progress. - private let hasOperationInProgress: Bool private let logger: InternalLogger private let clock: SimpleClock private let contributors: [Contributor] @@ -67,7 +65,6 @@ internal actor RoomLifecycleManager { ) async { await self.init( status: nil, - hasOperationInProgress: nil, pendingDiscontinuityEvents: [:], contributors: contributors, logger: logger, @@ -78,7 +75,6 @@ internal actor RoomLifecycleManager { #if DEBUG internal init( testsOnly_status status: Status? = nil, - testsOnly_hasOperationInProgress hasOperationInProgress: Bool? = nil, testsOnly_pendingDiscontinuityEvents pendingDiscontinuityEvents: [Contributor.ID: [ARTErrorInfo]]? = nil, contributors: [Contributor], logger: InternalLogger, @@ -86,7 +82,6 @@ internal actor RoomLifecycleManager { ) async { await self.init( status: status, - hasOperationInProgress: hasOperationInProgress, pendingDiscontinuityEvents: pendingDiscontinuityEvents, contributors: contributors, logger: logger, @@ -97,14 +92,12 @@ internal actor RoomLifecycleManager { private init( status: Status?, - hasOperationInProgress: Bool?, pendingDiscontinuityEvents: [Contributor.ID: [ARTErrorInfo]]?, contributors: [Contributor], logger: InternalLogger, clock: SimpleClock ) async { self.status = status ?? .initialized - self.hasOperationInProgress = hasOperationInProgress ?? false self.contributors = contributors contributorAnnotations = .init(contributors: contributors, pendingDiscontinuityEvents: pendingDiscontinuityEvents ?? [:]) self.logger = logger @@ -145,13 +138,13 @@ internal actor RoomLifecycleManager { internal enum Status: Equatable { case initialized - case attaching + case attaching(attachOperationID: UUID) case attached - case detaching + case detaching(detachOperationID: UUID) case detached case suspended(error: ARTErrorInfo) case failed(error: ARTErrorInfo) - case releasing + case releasing(releaseOperationID: UUID) case released internal var toRoomLifecycle: RoomLifecycle { @@ -176,6 +169,24 @@ internal actor RoomLifecycleManager { .released } } + + fileprivate var operationID: UUID? { + switch self { + case let .attaching(attachOperationID): + attachOperationID + case let .detaching(detachOperationID): + detachOperationID + case let .releasing(releaseOperationID): + releaseOperationID + case .suspended, + .initialized, + .attached, + .detached, + .failed, + .released: + nil + } + } } // MARK: - Types for contributor annotations @@ -359,10 +370,21 @@ internal actor RoomLifecycleManager { #endif } + // MARK: - Operation handling + + /// Whether the room lifecycle manager currently has a room lifecycle operation in progress. + /// + /// - Warning: I haven’t yet figured out the exact meaning of “has an operation in progress” — at what point is an operation considered to be no longer in progress? Is it the point at which the operation has updated the manager’s status to one that no longer indicates an in-progress operation (this is the meaning currently used by `hasOperationInProgress`)? Or is it the point at which the `perform*Operation` method for that operation exits? Does it matter? I’ve chosen to not think about this very much right now, but might need to revisit. See TODO against `emitPendingDiscontinuityEvents` in `performDetachOperation` for an example of something where these two notions of “has an operation in progress” are not equivalent. + private var hasOperationInProgress: Bool { + status.operationID != nil + } + // MARK: - ATTACH operation /// Implements CHA-RL1’s `ATTACH` operation. internal func performAttachOperation() async throws { + let operationID = UUID() + switch status { case .attached: // CHA-RL1a @@ -378,7 +400,7 @@ internal actor RoomLifecycleManager { } // CHA-RL1e - changeStatus(to: .attaching) + changeStatus(to: .attaching(attachOperationID: operationID)) // CHA-RL1f for contributor in contributors { @@ -418,6 +440,7 @@ internal actor RoomLifecycleManager { changeStatus(to: .attached) // CHA-RL1g2 + // TODO: It’s not clear to me whether this is considered to be part of the ATTACH operation or not; see the note on the ``hasOperationInProgress`` property await emitPendingDiscontinuityEvents() } @@ -456,6 +479,8 @@ internal actor RoomLifecycleManager { /// Implements CHA-RL2’s DETACH operation. internal func performDetachOperation() async throws { + let operationID = UUID() + switch status { case .detached: // CHA-RL2a @@ -474,7 +499,7 @@ internal actor RoomLifecycleManager { } // CHA-RL2e - changeStatus(to: .detaching) + changeStatus(to: .detaching(detachOperationID: operationID)) // CHA-RL2f var firstDetachError: Error? @@ -537,6 +562,8 @@ internal actor RoomLifecycleManager { /// Implements CHA-RL3’s RELEASE operation. internal func performReleaseOperation() async { + let operationID = UUID() + switch status { case .released: // CHA-RL3a @@ -550,7 +577,7 @@ internal actor RoomLifecycleManager { } // CHA-RL3c - changeStatus(to: .releasing) + changeStatus(to: .releasing(releaseOperationID: operationID)) // CHA-RL3d for contributor in contributors { diff --git a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift index 579f2a64..51487901 100644 --- a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift +++ b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift @@ -29,14 +29,12 @@ struct RoomLifecycleManagerTests { private func createManager( forTestingWhatHappensWhenCurrentlyIn status: RoomLifecycleManager.Status? = nil, - forTestingWhatHappensWhenHasOperationInProgress hasOperationInProgress: Bool? = nil, forTestingWhatHappensWhenHasPendingDiscontinuityEvents pendingDiscontinuityEvents: [MockRoomLifecycleContributor.ID: [ARTErrorInfo]]? = nil, contributors: [MockRoomLifecycleContributor] = [], clock: SimpleClock = MockSimpleClock() ) async -> RoomLifecycleManager { await .init( testsOnly_status: status, - testsOnly_hasOperationInProgress: hasOperationInProgress, testsOnly_pendingDiscontinuityEvents: pendingDiscontinuityEvents, contributors: contributors, logger: TestLogger(), @@ -99,7 +97,9 @@ struct RoomLifecycleManagerTests { @Test func attach_whenReleasing() async throws { // Given: A RoomLifecycleManager in the RELEASING state - let manager = await createManager(forTestingWhatHappensWhenCurrentlyIn: .releasing) + let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .releasing(releaseOperationID: UUID() /* arbitrary */ ) + ) // When: `performAttachOperation()` is called on the lifecycle manager // Then: It throws a roomIsReleasing error @@ -392,7 +392,9 @@ struct RoomLifecycleManagerTests { @Test func detach_whenReleasing() async throws { // Given: A RoomLifecycleManager in the RELEASING state - let manager = await createManager(forTestingWhatHappensWhenCurrentlyIn: .releasing) + let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .releasing(releaseOperationID: UUID() /* arbitrary */ ) + ) // When: `performDetachOperation()` is called on the lifecycle manager // Then: It throws a roomIsReleasing error @@ -751,7 +753,10 @@ struct RoomLifecycleManagerTests { func contributorUpdate_withResumedFalse_withOperationInProgress_recordsPendingDiscontinuityEvent() async throws { // Given: A RoomLifecycleManager, with a room lifecycle operation in progress let contributor = createContributor() - let manager = await createManager(forTestingWhatHappensWhenHasOperationInProgress: true, contributors: [contributor]) + let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .attaching(attachOperationID: UUID()), // case and ID arbitrary, just care that an operation is in progress + contributors: [contributor] + ) // When: A contributor emits an UPDATE event with `resumed` flag set to false let contributorStateChange = ARTChannelStateChange( @@ -779,7 +784,10 @@ struct RoomLifecycleManagerTests { func contributorUpdate_withResumedTrue_withNoOperationInProgress_emitsDiscontinuityEvent() async throws { // Given: A RoomLifecycleManager, with no room lifecycle operation in progress let contributor = createContributor() - let manager = await createManager(forTestingWhatHappensWhenHasOperationInProgress: false, contributors: [contributor]) + let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .initialized, // case arbitrary, just care that no operation is in progress + contributors: [contributor] + ) // When: A contributor emits an UPDATE event with `resumed` flag set to false let contributorStateChange = ARTChannelStateChange( @@ -807,7 +815,10 @@ struct RoomLifecycleManagerTests { func contributorAttachEvent_withResumeFalse_withOperationInProgress_recordsPendingDiscontinuityEvent() async throws { // Given: A RoomLifecycleManager, with a room lifecycle operation in progress let contributor = createContributor() - let manager = await createManager(forTestingWhatHappensWhenHasOperationInProgress: true, contributors: [contributor]) + let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .attaching(attachOperationID: UUID()), // case and ID arbitrary, just care that an operation is in progress + contributors: [contributor] + ) // When: A contributor emits an ATTACHED event with `resumed` flag set to false let contributorStateChange = ARTChannelStateChange( @@ -839,7 +850,10 @@ struct RoomLifecycleManagerTests { createContributor(detachBehavior: .success), createContributor(detachBehavior: .success), ] - let manager = await createManager(forTestingWhatHappensWhenHasOperationInProgress: false, contributors: contributors) + let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .initialized, // case arbitrary, just care that no operation is in progress + contributors: contributors + ) let roomStatusSubscription = await manager.onChange(bufferingPolicy: .unbounded) async let failedStatusChange = roomStatusSubscription.failedElements().first { _ in true } @@ -879,7 +893,6 @@ struct RoomLifecycleManagerTests { let manager = await createManager( forTestingWhatHappensWhenCurrentlyIn: .initialized, // arbitrary non-ATTACHED - forTestingWhatHappensWhenHasOperationInProgress: false, contributors: contributors ) @@ -911,10 +924,9 @@ struct RoomLifecycleManagerTests { createContributor(initialState: .detached), ] - let initialManagerStatus = RoomLifecycleManager.Status.initialized // arbitrary non-ATTACHED + let initialManagerStatus = RoomLifecycleManager.Status.detached // arbitrary non-ATTACHED, no-operation-in-progress let manager = await createManager( forTestingWhatHappensWhenCurrentlyIn: initialManagerStatus, - forTestingWhatHappensWhenHasOperationInProgress: false, contributors: contributors ) @@ -940,7 +952,10 @@ struct RoomLifecycleManagerTests { func contributorSuspendedEvent_withNoOperationInProgress() async throws { // Given: A RoomLifecycleManager with no lifecycle operation in progress let contributor = createContributor() - let manager = await createManager(forTestingWhatHappensWhenHasOperationInProgress: false, contributors: [contributor]) + let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .initialized, // case arbitrary, just care that no operation is in progress + contributors: [contributor] + ) let roomStatusSubscription = await manager.onChange(bufferingPolicy: .unbounded) async let maybeSuspendedRoomStatusChange = roomStatusSubscription.suspendedElements().first { _ in true } From e3dcfe11d40d24d89f243efec652c3175490b84a Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 10 Oct 2024 13:46:32 -0300 Subject: [PATCH 07/11] Allow a lifecycle operation to await another one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’ll use this mechanism to implement CHA-RL1d and CHA-RL3c for #52. --- Sources/AblyChat/RoomLifecycleManager.swift | 96 ++++++++++++++++++++- 1 file changed, 92 insertions(+), 4 deletions(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index f5fd3da2..e5f61ad2 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -55,6 +55,7 @@ internal actor RoomLifecycleManager { private var listenForStateChangesTask: Task! // TODO: clean up old subscriptions (https://github.com/ably-labs/ably-chat-swift/issues/36) private var subscriptions: [Subscription] = [] + private var operationResultContinuations = OperationResultContinuations() // MARK: - Initializers and `deinit` @@ -374,17 +375,96 @@ internal actor RoomLifecycleManager { /// Whether the room lifecycle manager currently has a room lifecycle operation in progress. /// - /// - Warning: I haven’t yet figured out the exact meaning of “has an operation in progress” — at what point is an operation considered to be no longer in progress? Is it the point at which the operation has updated the manager’s status to one that no longer indicates an in-progress operation (this is the meaning currently used by `hasOperationInProgress`)? Or is it the point at which the `perform*Operation` method for that operation exits? Does it matter? I’ve chosen to not think about this very much right now, but might need to revisit. See TODO against `emitPendingDiscontinuityEvents` in `performDetachOperation` for an example of something where these two notions of “has an operation in progress” are not equivalent. + /// - Warning: I haven’t yet figured out the exact meaning of “has an operation in progress” — at what point is an operation considered to be no longer in progress? Is it the point at which the operation has updated the manager’s status to one that no longer indicates an in-progress operation (this is the meaning currently used by `hasOperationInProgress`)? Or is it the point at which the `bodyOf*Operation` method for that operation exits (i.e. the point at which ``performAnOperation(_:)`` considers the operation to have completed)? Does it matter? I’ve chosen to not think about this very much right now, but might need to revisit. See TODO against `emitPendingDiscontinuityEvents` in `bodyOfDetachOperation` for an example of something where these two notions of “has an operation in progress” are not equivalent. private var hasOperationInProgress: Bool { status.operationID != nil } + /// Stores bookkeeping information needed for allowing one operation to await the result of another. + private struct OperationResultContinuations { + typealias Continuation = CheckedContinuation + + private var operationResultContinuationsByOperationID: [UUID: [Continuation]] = [:] + + mutating func addContinuation(_ continuation: Continuation, forResultOfOperationWithID operationID: UUID) { + operationResultContinuationsByOperationID[operationID, default: []].append(continuation) + } + + mutating func removeContinuationsForResultOfOperationWithID(_ waitedOperationID: UUID) -> [Continuation] { + operationResultContinuationsByOperationID.removeValue(forKey: waitedOperationID) ?? [] + } + } + + /// Waits for the operation with ID `waitedOperationID` to complete, re-throwing any error thrown by that operation. + /// + /// Note that this method currently treats all waited operations as throwing. If you wish to wait for an operation that you _know_ to be non-throwing (which the RELEASE operation currently is) then you’ll need to call this method with `try!` or equivalent. (It might be possible to improve this in the future, but I didn’t want to put much time into figuring it out.) + /// + /// It is guaranteed that if you call this method from a manager-isolated method, and subsequently call ``operationWithID(_:,didCompleteWithResult:)`` from another manager-isolated method, then the call to this method will return. + /// + /// - Parameters: + /// - waitedOperationID: The ID of the operation whose completion will be awaited. + /// - waitingOperationID: The ID of the operation which is awaiting this result. Only used for logging. + private func waitForCompletionOfOperationWithID( + _ waitedOperationID: UUID, + waitingOperationID: UUID + ) async throws { + logger.log(message: "Operation \(waitingOperationID) started waiting for result of operation \(waitedOperationID)", level: .debug) + + do { + try await withCheckedThrowingContinuation { (continuation: OperationResultContinuations.Continuation) in + // My “it is guaranteed” in the documentation for this method is really more of an “I hope that”, because it’s based on my pretty vague understanding of Swift concurrency concepts; namely, I believe that if you call this manager-isolated `async` method from another manager-isolated method, the initial synchronous part of this method — in particular the call to `addContinuation` below — will occur _before_ the call to this method suspends. (I think this can be roughly summarised as “calls to async methods on self don’t do actor hopping” but I could be completely misusing a load of Swift concurrency vocabulary there.) + operationResultContinuations.addContinuation(continuation, forResultOfOperationWithID: waitedOperationID) + } + + logger.log(message: "Operation \(waitingOperationID) completed waiting for result of operation \(waitedOperationID), which completed successfully", level: .debug) + } catch { + logger.log(message: "Operation \(waitingOperationID) completed waiting for result of operation \(waitedOperationID), which threw error \(error)", level: .debug) + } + } + + /// Operations should call this when they have completed, in order to complete any waits initiated by ``waitForCompletionOfOperationWithID(_:waitingOperationID:)``. + private func operationWithID(_ operationID: UUID, didCompleteWithResult result: Result) { + logger.log(message: "Operation \(operationID) completed with result \(result)", level: .debug) + let continuationsToResume = operationResultContinuations.removeContinuationsForResultOfOperationWithID(operationID) + + for continuation in continuationsToResume { + continuation.resume(with: result) + } + } + + /// Executes a function that represents a room lifecycle operation. + /// + /// - Note: Note that `RoomLifecycleManager` does not implement any sort of mutual exclusion mechanism that _enforces_ that one room lifecycle operation must wait for another (e.g. it is _not_ a queue); each operation needs to implement its own logic for whether it should proceed in the presence of other in-progress operations. + /// + /// - Parameters: + /// - body: The implementation of the operation to be performed. Once this function returns or throws an error, the operation is considered to have completed, and any waits for this operation’s completion initiated via ``waitForCompletionOfOperationWithID(_:waitingOperationID:)`` will complete. + private func performAnOperation(_ body: (UUID) async throws(Failure) -> Void) async throws(Failure) { + let operationID = UUID() + logger.log(message: "Performing operation \(operationID)", level: .debug) + let result: Result + do { + // My understanding (based on what the compiler allows me to do, and a vague understanding of how actors work) is that inside this closure you can write code as if it were a method on the manager itself — i.e. with synchronous access to the manager’s state. But I currently lack the Swift concurrency vocabulary to explain exactly why this is the case. + try await body(operationID) + result = .success(()) + } catch { + result = .failure(error) + } + + operationWithID(operationID, didCompleteWithResult: result.mapError { $0 }) + + try result.get() + } + // MARK: - ATTACH operation /// Implements CHA-RL1’s `ATTACH` operation. internal func performAttachOperation() async throws { - let operationID = UUID() + try await performAnOperation { operationID in + try await bodyOfAttachOperation(operationID: operationID) + } + } + private func bodyOfAttachOperation(operationID: UUID) async throws { switch status { case .attached: // CHA-RL1a @@ -479,8 +559,12 @@ internal actor RoomLifecycleManager { /// Implements CHA-RL2’s DETACH operation. internal func performDetachOperation() async throws { - let operationID = UUID() + try await performAnOperation { operationID in + try await bodyOfDetachOperation(operationID: operationID) + } + } + private func bodyOfDetachOperation(operationID: UUID) async throws { switch status { case .detached: // CHA-RL2a @@ -562,8 +646,12 @@ internal actor RoomLifecycleManager { /// Implements CHA-RL3’s RELEASE operation. internal func performReleaseOperation() async { - let operationID = UUID() + await performAnOperation { operationID in + await bodyOfReleaseOperation(operationID: operationID) + } + } + private func bodyOfReleaseOperation(operationID: UUID) async { switch status { case .released: // CHA-RL3a From 57e1459c37b89c57316a816bcf89392ecbb8894a Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 10 Oct 2024 16:39:05 -0300 Subject: [PATCH 08/11] Expose lifecycle operation waits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- Sources/AblyChat/RoomLifecycleManager.swift | 28 +++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index e5f61ad2..f0feeb7d 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -395,6 +395,27 @@ internal actor RoomLifecycleManager { } } + #if DEBUG + /// The manager emits an `OperationWaitEvent` each time one room lifecycle operation is going to wait for another to complete. These events are emitted to support testing of the manager; see ``testsOnly_subscribeToOperationWaitEvents``. + internal struct OperationWaitEvent: Equatable { + /// The ID of the operation which initiated the wait. It is waiting for the operation with ID ``waitedOperationID`` to complete. + internal var waitingOperationID: UUID + /// The ID of the operation whose completion will be awaited. + internal var waitedOperationID: UUID + } + + // TODO: clean up old subscriptions (https://github.com/ably-labs/ably-chat-swift/issues/36) + /// Supports the ``testsOnly_subscribeToOperationWaitEvents()`` method. + private var operationWaitEventSubscriptions: [Subscription] = [] + + /// Returns a subscription which emits an event each time one room lifecycle operation is going to wait for another to complete. + internal func testsOnly_subscribeToOperationWaitEvents() -> Subscription { + let subscription = Subscription(bufferingPolicy: .unbounded) + operationWaitEventSubscriptions.append(subscription) + return subscription + } + #endif + /// Waits for the operation with ID `waitedOperationID` to complete, re-throwing any error thrown by that operation. /// /// Note that this method currently treats all waited operations as throwing. If you wish to wait for an operation that you _know_ to be non-throwing (which the RELEASE operation currently is) then you’ll need to call this method with `try!` or equivalent. (It might be possible to improve this in the future, but I didn’t want to put much time into figuring it out.) @@ -414,6 +435,13 @@ internal actor RoomLifecycleManager { try await withCheckedThrowingContinuation { (continuation: OperationResultContinuations.Continuation) in // My “it is guaranteed” in the documentation for this method is really more of an “I hope that”, because it’s based on my pretty vague understanding of Swift concurrency concepts; namely, I believe that if you call this manager-isolated `async` method from another manager-isolated method, the initial synchronous part of this method — in particular the call to `addContinuation` below — will occur _before_ the call to this method suspends. (I think this can be roughly summarised as “calls to async methods on self don’t do actor hopping” but I could be completely misusing a load of Swift concurrency vocabulary there.) operationResultContinuations.addContinuation(continuation, forResultOfOperationWithID: waitedOperationID) + + #if DEBUG + let operationWaitEvent = OperationWaitEvent(waitingOperationID: waitingOperationID, waitedOperationID: waitedOperationID) + for subscription in operationWaitEventSubscriptions { + subscription.emit(operationWaitEvent) + } + #endif } logger.log(message: "Operation \(waitingOperationID) completed waiting for result of operation \(waitedOperationID), which completed successfully", level: .debug) From 7a3debf69912607d1924384a3831487c10860d7f Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 10 Oct 2024 16:46:36 -0300 Subject: [PATCH 09/11] Allow tests to force a lifecycle operation ID Will use this in some upcoming tests. Intended for use with the API added in 57e1459. --- Sources/AblyChat/RoomLifecycleManager.swift | 29 +++++++++++++++------ 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index f0feeb7d..711eac6e 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -465,9 +465,13 @@ internal actor RoomLifecycleManager { /// - Note: Note that `RoomLifecycleManager` does not implement any sort of mutual exclusion mechanism that _enforces_ that one room lifecycle operation must wait for another (e.g. it is _not_ a queue); each operation needs to implement its own logic for whether it should proceed in the presence of other in-progress operations. /// /// - Parameters: + /// - forcedOperationID: Forces the operation to have a given ID. In combination with the ``testsOnly_subscribeToOperationWaitEvents`` API, this allows tests to verify that one test-initiated operation is waiting for another test-initiated operation. /// - body: The implementation of the operation to be performed. Once this function returns or throws an error, the operation is considered to have completed, and any waits for this operation’s completion initiated via ``waitForCompletionOfOperationWithID(_:waitingOperationID:)`` will complete. - private func performAnOperation(_ body: (UUID) async throws(Failure) -> Void) async throws(Failure) { - let operationID = UUID() + private func performAnOperation( + forcingOperationID forcedOperationID: UUID?, + _ body: (UUID) async throws(Failure) -> Void + ) async throws(Failure) { + let operationID = forcedOperationID ?? UUID() logger.log(message: "Performing operation \(operationID)", level: .debug) let result: Result do { @@ -486,8 +490,11 @@ internal actor RoomLifecycleManager { // MARK: - ATTACH operation /// Implements CHA-RL1’s `ATTACH` operation. - internal func performAttachOperation() async throws { - try await performAnOperation { operationID in + /// + /// - Parameters: + /// - forcedOperationID: Allows tests to force the operation to have a given ID. In combination with the ``testsOnly_subscribeToOperationWaitEvents`` API, this allows tests to verify that one test-initiated operation is waiting for another test-initiated operation. + internal func performAttachOperation(testsOnly_forcingOperationID forcedOperationID: UUID? = nil) async throws { + try await performAnOperation(forcingOperationID: forcedOperationID) { operationID in try await bodyOfAttachOperation(operationID: operationID) } } @@ -586,8 +593,11 @@ internal actor RoomLifecycleManager { // MARK: - DETACH operation /// Implements CHA-RL2’s DETACH operation. - internal func performDetachOperation() async throws { - try await performAnOperation { operationID in + /// + /// - Parameters: + /// - forcedOperationID: Allows tests to force the operation to have a given ID. In combination with the ``testsOnly_subscribeToOperationWaitEvents`` API, this allows tests to verify that one test-initiated operation is waiting for another test-initiated operation. + internal func performDetachOperation(testsOnly_forcingOperationID forcedOperationID: UUID? = nil) async throws { + try await performAnOperation(forcingOperationID: forcedOperationID) { operationID in try await bodyOfDetachOperation(operationID: operationID) } } @@ -673,8 +683,11 @@ internal actor RoomLifecycleManager { // MARK: - RELEASE operation /// Implements CHA-RL3’s RELEASE operation. - internal func performReleaseOperation() async { - await performAnOperation { operationID in + /// + /// - Parameters: + /// - forcedOperationID: Allows tests to force the operation to have a given ID. In combination with the ``testsOnly_subscribeToOperationWaitEvents`` API, this allows tests to verify that one test-initiated operation is waiting for another test-initiated operation. + internal func performReleaseOperation(testsOnly_forcingOperationID forcedOperationID: UUID? = nil) async { + await performAnOperation(forcingOperationID: forcedOperationID) { operationID in await bodyOfReleaseOperation(operationID: operationID) } } From 02db99f466b2aaa221b60e971229c01d93e85ff5 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 10 Oct 2024 15:46:44 -0300 Subject: [PATCH 10/11] Implement CHA-RL1d (From the same spec as referenced in 25e5052.) --- Sources/AblyChat/RoomLifecycleManager.swift | 5 ++ .../RoomLifecycleManagerTests.swift | 48 +++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index 711eac6e..89b9b7aa 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -514,6 +514,11 @@ internal actor RoomLifecycleManager { break } + // CHA-RL1d + if let currentOperationID = status.operationID { + try? await waitForCompletionOfOperationWithID(currentOperationID, waitingOperationID: operationID) + } + // CHA-RL1e changeStatus(to: .attaching(attachOperationID: operationID)) diff --git a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift index 51487901..45d3be84 100644 --- a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift +++ b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift @@ -125,6 +125,54 @@ struct RoomLifecycleManagerTests { } } + // @spec CHA-RL1d + @Test + func attach_ifOtherOperationInProgress_waitsForItToComplete() async throws { + // Given: A RoomLifecycleManager with a DETACH lifecycle operation in progress (the fact that it is a DETACH is not important; it is just an operation whose execution it is easy to prolong and subsequently complete, which is helpful for this test) + let contributorDetachOperation = SignallableChannelOperation() + let manager = await createManager( + contributors: [ + createContributor( + // Arbitrary, allows the ATTACH to eventually complete + attachBehavior: .complete(.success), + // This allows us to prolong the execution of the DETACH triggered in (1) + detachBehavior: contributorDetachOperation.behavior + ), + ] + ) + + let detachOperationID = UUID() + let attachOperationID = UUID() + + let statusChangeSubscription = await manager.onChange(bufferingPolicy: .unbounded) + // Wait for the manager to enter DETACHING; this our sign that the DETACH operation triggered in (1) has started + async let detachingStatusChange = statusChangeSubscription.first { $0.current == .detaching } + + // (1) This is the "DETACH lifecycle operation in progress" mentioned in Given + async let _ = manager.performDetachOperation(testsOnly_forcingOperationID: detachOperationID) + _ = await detachingStatusChange + + let operationWaitEventSubscription = await manager.testsOnly_subscribeToOperationWaitEvents() + async let attachedWaitingForDetachedEvent = operationWaitEventSubscription.first { operationWaitEvent in + operationWaitEvent == .init(waitingOperationID: attachOperationID, waitedOperationID: detachOperationID) + } + + // When: `performAttachOperation()` is called on the lifecycle manager + async let attachResult: Void = manager.performAttachOperation(testsOnly_forcingOperationID: attachOperationID) + + // Then: + // - the manager informs us that the ATTACH operation is waiting for the DETACH operation to complete + // - when the DETACH completes, the ATTACH operation proceeds (which we check here by verifying that it eventually completes) — note that (as far as I can tell) there is no way to test that the ATTACH operation would have proceeded _only if_ the DETACH had completed; the best we can do is allow the manager to tell us that that this is indeed what it’s doing (which is what we check for in the previous bullet) + + _ = try #require(await attachedWaitingForDetachedEvent) + + // Allow the DETACH to complete + contributorDetachOperation.complete(result: .success /* arbitrary */ ) + + // Check that ATTACH completes + try await attachResult + } + // @spec CHA-RL1e @Test func attach_transitionsToAttaching() async throws { From ce376067aba0635385416459407e13cdfdc39e57 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Mon, 14 Oct 2024 11:56:29 -0300 Subject: [PATCH 11/11] Implement CHA-RL3c (From the same spec as referenced in 25e5052.) Resolves #52. --- Sources/AblyChat/RoomLifecycleManager.swift | 7 ++- .../RoomLifecycleManagerTests.swift | 46 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index 89b9b7aa..cb1ef86a 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -706,7 +706,12 @@ internal actor RoomLifecycleManager { // CHA-RL3b changeStatus(to: .released) return - case .releasing, .initialized, .attached, .attaching, .detaching, .suspended, .failed: + case let .releasing(releaseOperationID): + // CHA-RL3c + // See note on waitForCompletionOfOperationWithID for the current need for this force try + // swiftlint:disable:next force_try + return try! await waitForCompletionOfOperationWithID(releaseOperationID, waitingOperationID: operationID) + case .initialized, .attached, .attaching, .detaching, .suspended, .failed: break } diff --git a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift index 45d3be84..0a5b25fe 100644 --- a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift +++ b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift @@ -650,6 +650,52 @@ struct RoomLifecycleManagerTests { #expect(await contributor.channel.detachCallCount == 0) } + // @specPartial CHA-RL3c - This is marked as specPartial because at time of writing the spec point CHA-RL3c has been accidentally duplicated to specify two separate behaviours. TODO: change this one to @spec once spec fixed (see discussion in https://github.com/ably/specification/pull/200#discussion_r1763770348) + @Test + func release_whenReleasing() async throws { + // Given: A RoomLifecycleManager with a RELEASE lifecycle operation in progress, and hence in the RELEASING state + let contributorDetachOperation = SignallableChannelOperation() + let contributor = createContributor( + // This allows us to prolong the execution of the RELEASE triggered in (1) + detachBehavior: contributorDetachOperation.behavior + ) + let manager = await createManager(contributors: [contributor]) + + let firstReleaseOperationID = UUID() + let secondReleaseOperationID = UUID() + + let statusChangeSubscription = await manager.onChange(bufferingPolicy: .unbounded) + // Wait for the manager to enter RELEASING; this our sign that the DETACH operation triggered in (1) has started + async let releasingStatusChange = statusChangeSubscription.first { $0.current == .releasing } + + // (1) This is the "RELEASE lifecycle operation in progress" mentioned in Given + async let _ = manager.performReleaseOperation(testsOnly_forcingOperationID: firstReleaseOperationID) + _ = await releasingStatusChange + + let operationWaitEventSubscription = await manager.testsOnly_subscribeToOperationWaitEvents() + async let secondReleaseWaitingForFirstReleaseEvent = operationWaitEventSubscription.first { operationWaitEvent in + operationWaitEvent == .init(waitingOperationID: secondReleaseOperationID, waitedOperationID: firstReleaseOperationID) + } + + // When: `performReleaseOperation()` is called on the lifecycle manager + async let secondReleaseResult: Void = manager.performReleaseOperation(testsOnly_forcingOperationID: secondReleaseOperationID) + + // Then: + // - the manager informs us that the second RELEASE operation is waiting for first RELEASE operation to complete + // - when the first RELEASE completes, the second RELEASE operation also completes + // - the second RELEASE operation does not perform any side-effects (which we check here by confirming that the CHA-RL3d contributor detach only happened once, i.e. was only performed by the first RELEASE operation) + + _ = try #require(await secondReleaseWaitingForFirstReleaseEvent) + + // Allow the first RELEASE to complete + contributorDetachOperation.complete(result: .success) + + // Check that the second RELEASE completes + await secondReleaseResult + + #expect(await contributor.channel.detachCallCount == 1) + } + // @specPartial CHA-RL3c - Haven’t implemented the part that refers to "transient disconnect timeouts"; TODO do this (https://github.com/ably-labs/ably-chat-swift/issues/48) @Test func release_transitionsToReleasing() async throws {