From db9d4b50f23b70a89c473da6ca08dad536975233 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 4 Dec 2024 11:22:40 -0300 Subject: [PATCH 1/2] Fix test comment language for consistency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don’t use this “will” language elsewhere. --- Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift b/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift index 3d1dedc8..438aa050 100644 --- a/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift +++ b/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift @@ -488,8 +488,8 @@ struct DefaultRoomLifecycleManagerTests { // Then: // - // - the lifecycle manager will call `detach` on contributors 0 and 2 - // - the lifecycle manager will not call `detach` on contributor 1 + // - the lifecycle manager calls `detach` on contributors 0 and 2 + // - the lifecycle manager does not call `detach` on contributor 1 #expect(await contributors[0].channel.detachCallCount > 0) #expect(await contributors[2].channel.detachCallCount > 0) #expect(await contributors[1].channel.detachCallCount == 0) @@ -528,7 +528,7 @@ struct DefaultRoomLifecycleManagerTests { // When: `performAttachOperation()` is called on the lifecycle manager try? await manager.performAttachOperation() - // Then: the lifecycle manager will call `detach` twice on contributor 0 (i.e. it will retry the failed detach) + // Then: the lifecycle manager calls `detach` twice on contributor 0 (i.e. it retries the failed detach) #expect(await contributors[0].channel.detachCallCount == 2) } From 77d0791d8b94a533367c95f2dd57dbee20a6cc4c Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Tue, 3 Dec 2024 17:23:34 -0300 Subject: [PATCH 2/2] =?UTF-8?q?Implement=20=E2=80=9Casynchronously?= =?UTF-8?q?=E2=80=9D=20part=20of=20CHA-RL1h5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This implementation reflects my suggested spec changes [1] which aim to preserve lifecycle operation atomicity by introducing a new RUNDOWN operation: > The `ATTACH` operation ends in CHA-RL1h4, implying that the CHA-RL1h5 > asynchronous detach happens _outside of any room lifecycle operation_. > This means that another room lifecycle operation could run at the same > time as this detach operation, which doesn't seem intentional. > > I looked at the JS implementation [2] and it seems that it keeps the > lifecycle manager’s mutex locked during this "rundown" (as it calls the > CHA-RL1h5 detach operation). But this is not implied in the spec. I > think that to translate this behaviour to the spec, which implements > mutual exclusion through lifecycle operations, we should do something > analogous to the `RETRY` operation; that is, define a new internal-only > room lifecycle operation (I’ll call it `RUNDOWN` for want of a better > term), which is scheduled by CHA-RL1h5 and which: > > - performs the detach-all-non-`FAILED`-contributors behaviour of CHA-RL1h5 > - implements the retry behaviour of CHA-RL1h6 Resolves #119. [1] https://github.com/ably/specification/issues/253 [2] https://github.com/ably/ably-chat-js/blob/e8380583424a83f7151405cc0716e01302295eb6/src/core/room-lifecycle-manager.ts#L506-L509 --- Sources/AblyChat/RoomLifecycleManager.swift | 101 +++++++++--- .../DefaultRoomLifecycleManagerTests.swift | 150 +++++++++++++----- 2 files changed, 182 insertions(+), 69 deletions(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index 8c61522e..f917e891 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -192,6 +192,9 @@ internal actor DefaultRoomLifecycleManager, error: ARTErrorInfo) case suspended(retryOperationID: UUID, error: ARTErrorInfo) + // `rundownOperationTask` is exposed so that tests can wait for the triggered RUNDOWN operation to complete. + case failedAwaitingStartOfRundownOperation(rundownOperationTask: Task, error: ARTErrorInfo) + case failedAndPerformingRundownOperation(rundownOperationID: UUID, error: ARTErrorInfo) case failed(error: ARTErrorInfo) case releasing(releaseOperationID: UUID) case released @@ -216,6 +219,10 @@ internal actor DefaultRoomLifecycleManager 0) - #expect(await contributors[2].channel.detachCallCount > 0) - #expect(await contributors[1].channel.detachCallCount == 0) - } - - // @spec CHA-RL1h6 - @Test - func attach_whenChannelDetachTriggered_ifADetachFailsItIsRetriedUntilSuccess() async throws { - // Given: A room with the following contributors, in the following order: - // - // 0. a channel: - // - for whom calling `attach` will complete successfully, putting it in the ATTACHED state (i.e. an arbitrarily-chosen state that is not FAILED) - // - and for whom subsequently calling `detach` will fail on the first attempt and succeed on the second - // 1. a channel for whom calling `attach` will fail, putting it in the FAILED state (we won’t make any assertions about this channel; it’s just to trigger the room’s channel detach behaviour) - - let detachResult = { @Sendable (callCount: Int) async -> MockRoomLifecycleContributorChannel.AttachOrDetachBehavior in - if callCount == 1 { - return .failure(.create(withCode: 123, message: "")) + // - the lifecycle manager schedules a RUNDOWN operation + // - when we wait for this RUNDOWN operation to complete, we confirm that it has occured by checking for its side effects, namely that: + // - the lifecycle manager has called `detach` on contributors 0 and 2 + // - the lifecycle manager has not called `detach` on contributor 1 + + let rundownOperationTask = try #require(await managerStatusSubscription.compactMap { managerStatusChange in + if case let .failedAwaitingStartOfRundownOperation(rundownOperationTask, _) = managerStatusChange.current { + rundownOperationTask } else { - return .success + nil } } + .first { _ in true }) - let contributors = [ - createContributor( - attachBehavior: .completeAndChangeState(.success, newState: .attached), - detachBehavior: .fromFunction(detachResult) - ), - createContributor( - attachBehavior: .completeAndChangeState(.failure(.create(withCode: 123, message: "")), newState: .failed) - ), - ] - - let manager = await createManager(contributors: contributors) - - // When: `performAttachOperation()` is called on the lifecycle manager - try? await manager.performAttachOperation() + _ = await rundownOperationTask.value - // Then: the lifecycle manager calls `detach` twice on contributor 0 (i.e. it retries the failed detach) - #expect(await contributors[0].channel.detachCallCount == 2) + #expect(await contributors[0].channel.detachCallCount > 0) + #expect(await contributors[2].channel.detachCallCount > 0) + #expect(await contributors[1].channel.detachCallCount == 0) } // MARK: - DETACH operation @@ -1306,7 +1285,7 @@ struct DefaultRoomLifecycleManagerTests { let contributors = [ createContributor( attachBehavior: .success, - detachBehavior: .success, // So that the detach performed by CHA-RL1h5’s rollback of the attachment cycle succeeds + detachBehavior: .success, // So that the detach performed by CHA-RL1h5’s triggered RUNDOWN succeeds subscribeToStateBehavior: .addSubscriptionAndEmitStateChange( .init( current: .attached, @@ -1322,7 +1301,7 @@ struct DefaultRoomLifecycleManagerTests { ), createContributor( attachBehavior: .success, - detachBehavior: .success // So that the CHA-RL5a detachment cycle completes (not related to this test) and so that the detach performed by CHA-RL1h5’s rollback of the attachment cycle succeeds + detachBehavior: .success // So that the CHA-RL5a detachment cycle completes (not related to this test) and so that the detach performed by CHA-RL1h5’s triggered RUNDOWN succeeds ), ] @@ -1333,22 +1312,105 @@ struct DefaultRoomLifecycleManagerTests { let roomStatusSubscription = await manager.onRoomStatusChange(bufferingPolicy: .unbounded) async let failedStatusChange = roomStatusSubscription.failedElements().first { _ in true } + let managerStatusSubscription = await manager.testsOnly_onStatusChange() + // When: `performRetryOperation(triggeredByContributor:errorForSuspendedStatus:)` is called on the manager await manager.performRetryOperation(triggeredByContributor: contributors[0], errorForSuspendedStatus: .createUnknownError() /* arbitrary */ ) // Then: The room performs a CHA-RL1e attachment cycle (we sufficiently satisfy ourselves of this fact by checking that it’s attempted to attach all of the channels up to and including the one for which attachment fails, and subsequently detached all channels except for the FAILED one), transitions to FAILED, and the RETRY operation completes + + // We first wait for CHA-RLh5’s triggered RUNDOWN operation to complete, so that we can make assertions about its side effects + let rundownOperationTask = try #require(await managerStatusSubscription.compactMap { managerStatusChange in + if case let .failedAwaitingStartOfRundownOperation(rundownOperationTask, _) = managerStatusChange.current { + rundownOperationTask + } else { + nil + } + } + .first { _ in true }) + + _ = await rundownOperationTask.value + #expect(await contributors[0].channel.attachCallCount == 1) #expect(await contributors[1].channel.attachCallCount == 1) #expect(await contributors[2].channel.attachCallCount == 0) - #expect(await contributors[0].channel.detachCallCount == 1) // from CHA-RL1h5’s rollback of the attachment cycle + #expect(await contributors[0].channel.detachCallCount == 1) // from CHA-RL1h5’s triggered RUNDOWN #expect(await contributors[1].channel.detachCallCount == 1) // from CHA-RL5a detachment cycle - #expect(await contributors[2].channel.detachCallCount == 2) // from CHA-RL5a detachment cycle and CHA-RL1h5’s rollback of the attachment cycle + #expect(await contributors[2].channel.detachCallCount == 2) // from CHA-RL5a detachment cycle and CHA-RL1h5’s triggered RUNDOWN _ = try #require(await failedStatusChange) #expect(await manager.roomStatus.isFailed) } + // MARK: - RUNDOWN operation + + // @specOneOf(2/2) CHA-RL1h5 - Tests the behaviour of the RUNDOWN operation - see TODO on `performRundownOperation` for my interpretation of CHA-RL1h5, in which I introduce RUNDOWN + @Test + func rundown_detachesAllNonFailedChannels() async throws { + // Given: A room with the following contributors, in the following order: + // + // 0. a channel in the ATTACHED state (i.e. an arbitrarily-chosen state that is not FAILED) + // 1. a channel in the FAILED state + // 2. a channel in the INITIALIZED state (another arbitrarily-chosen state that is not FAILED) + // + // for which, when `detach` is called on contributors 0 and 2 (i.e. the non-FAILED contributors), it completes successfully + let contributors = [ + createContributor( + initialState: .attached, + detachBehavior: .success + ), + createContributor( + initialState: .failed + ), + createContributor( + detachBehavior: .success + ), + ] + + let manager = await createManager(contributors: contributors) + + // When: `performRundownOperation()` is called on the lifecycle manager + await manager.performRundownOperation(errorForFailedStatus: .createUnknownError() /* arbitrary */ ) + + // Then: + // + // - the lifecycle manager calls `detach` on contributors 0 and 2 + // - the lifecycle manager does not call `detach` on contributor 1 + // - the call to `performRundownOperation()` completes + #expect(await contributors[0].channel.detachCallCount == 1) + #expect(await contributors[2].channel.detachCallCount == 1) + #expect(await contributors[1].channel.detachCallCount == 0) + } + + // @spec CHA-RL1h6 - see TODO on `performRundownOperation` for my interpretation of CHA-RL1h5, in which I introduce RUNDOWN + @Test + func rundown_ifADetachFailsItIsRetriedUntilSuccess() async throws { + // Given: A room with a contributor in the ATTACHED state (i.e. an arbitrarily-chosen state that is not FAILED) and for whom calling `detach` will fail on the first attempt and succeed on the second + + let detachResult = { @Sendable (callCount: Int) async -> MockRoomLifecycleContributorChannel.AttachOrDetachBehavior in + if callCount == 1 { + return .failure(.create(withCode: 123, message: "")) + } else { + return .success + } + } + + let contributors = [ + createContributor( + detachBehavior: .fromFunction(detachResult) + ), + ] + + let manager = await createManager(contributors: contributors) + + // When: `performRundownOperation()` is called on the lifecycle manager + await manager.performRundownOperation(errorForFailedStatus: .createUnknownError() /* arbitrary */ ) + + // Then: the lifecycle manager calls `detach` twice on the contributor (i.e. it retries the failed detach) + #expect(await contributors[0].channel.detachCallCount == 2) + } + // MARK: - Handling contributor UPDATE events // @spec CHA-RL4a1