From 8183e5a770c160f19657140ac6c9a7b52cee042d Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 16 Oct 2024 17:21:44 -0300 Subject: [PATCH] Clear transient disconnect timeouts per spec Based on the spec referenced in 25e5052. Resolves #48. --- Sources/AblyChat/RoomLifecycleManager.swift | 44 ++++++- .../RoomLifecycleManagerTests.swift | 118 ++++++++++++++++-- 2 files changed, 145 insertions(+), 17 deletions(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index fcf6d8aa..53e1a8c0 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -304,6 +304,7 @@ internal actor RoomLifecycleManager { /// - the manager has performed all status changes provoked by the state change /// - the manager has performed all contributor actions provoked by the state change, namely calls to ``RoomLifecycleContributorChannel.detach()`` or ``RoomLifecycleContributor.emitDiscontinuity(_:)`` /// - the manager has recorded all transient disconnect timeouts provoked by the state change (you can retrieve this information using ``testsOnly_hasTransientDisconnectTimeout(for:) or ``testsOnly_idOfTransientDisconnectTimeout(for:)``) + /// - the manager has performed all transient disconnect timeouts cancellations by the state change (you can retrieve this information using ``testsOnly_hasTransientDisconnectTimeout(for:) or ``testsOnly_idOfTransientDisconnectTimeout(for:)``) internal func testsOnly_subscribeToHandledContributorStateChanges() -> Subscription { let subscription = Subscription(bufferingPolicy: .unbounded) stateChangeHandledSubscriptions.append(subscription) @@ -318,6 +319,10 @@ internal actor RoomLifecycleManager { contributorAnnotations[contributor].hasTransientDisconnectTimeout } + internal var testsOnly_hasTransientDisconnectTimeoutForAnyContributor: Bool { + contributors.contains { testsOnly_hasTransientDisconnectTimeout(for: $0) } + } + internal func testsOnly_idOfTransientDisconnectTimeout(for contributor: Contributor) -> UUID? { contributorAnnotations[contributor].transientDisconnectTimeout?.id } @@ -364,11 +369,16 @@ internal actor RoomLifecycleManager { contributorAnnotations[contributor].pendingDiscontinuityEvents.append(reason) } - } 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) - changeStatus(to: .attached) + } else { + // CHA-RL4b7 (the second one with this identifier) + clearTransientDisconnectTimeouts(for: contributor) + + 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) + changeStatus(to: .attached) + } } } case .failed: @@ -379,6 +389,7 @@ internal actor RoomLifecycleManager { preconditionFailure("FAILED state change event should have a reason") } + clearTransientDisconnectTimeouts() changeStatus(to: .failed(error: reason)) // TODO: CHA-RL4b5 is a bit unclear about how to handle failure, and whether they can be detached concurrently (asked in https://github.com/ably/specification/pull/200/files#r1777471810) @@ -398,6 +409,8 @@ internal actor RoomLifecycleManager { preconditionFailure("SUSPENDED state change event should have a reason") } + clearTransientDisconnectTimeouts() + changeStatus(to: .suspended(error: reason)) } case .attaching: @@ -429,6 +442,22 @@ internal actor RoomLifecycleManager { #endif } + private func clearTransientDisconnectTimeouts(for contributor: Contributor) { + guard let transientDisconnectTimeout = contributorAnnotations[contributor].transientDisconnectTimeout else { + return + } + + logger.log(message: "Clearing transient disconnect timeout \(transientDisconnectTimeout.id) for \(contributor)", level: .debug) + transientDisconnectTimeout.task?.cancel() + contributorAnnotations[contributor].transientDisconnectTimeout = nil + } + + private func clearTransientDisconnectTimeouts() { + for contributor in contributors { + clearTransientDisconnectTimeouts(for: contributor) + } + } + // MARK: - Operation handling /// Whether the room lifecycle manager currently has a room lifecycle operation in progress. @@ -614,6 +643,9 @@ internal actor RoomLifecycleManager { } } + // CHA-RL1g3 + clearTransientDisconnectTimeouts() + // CHA-RL1g1 changeStatus(to: .attached) @@ -684,6 +716,7 @@ internal actor RoomLifecycleManager { } // CHA-RL2e + clearTransientDisconnectTimeouts() changeStatus(to: .detaching(detachOperationID: operationID)) // CHA-RL2f @@ -774,6 +807,7 @@ internal actor RoomLifecycleManager { } // CHA-RL3c + clearTransientDisconnectTimeouts() changeStatus(to: .releasing(releaseOperationID: operationID)) // CHA-RL3d diff --git a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift index 0d4078d4..f304f319 100644 --- a/Tests/AblyChatTests/RoomLifecycleManagerTests.swift +++ b/Tests/AblyChatTests/RoomLifecycleManagerTests.swift @@ -276,6 +276,23 @@ struct RoomLifecycleManagerTests { } } + // @spec CHA-RL1g3 + @Test + func attach_uponSuccess_clearsTransientDisconnectTimeouts() async throws { + // Given: A RoomLifecycleManager, all of whose contributors’ calls to `attach` succeed + let contributors = (1 ... 3).map { _ in createContributor(attachBehavior: .complete(.success)) } + let manager = await createManager( + forTestingWhatHappensWhenHasTransientDisconnectTimeoutForTheseContributorIDs: [contributors[1].id], + contributors: contributors + ) + + // When: `performAttachOperation()` is called on the lifecycle manager + try await manager.performAttachOperation() + + // Then: It clears all transient disconnect timeouts + #expect(await !manager.testsOnly_hasTransientDisconnectTimeoutForAnyContributor) + } + // @spec CHA-RL1h2 // @specOneOf(1/2) CHA-RL1h1 - tests that an error gets thrown when channel attach fails due to entering SUSPENDED (TODO: but I don’t yet fully understand the meaning of CHA-RL1h1; outstanding question https://github.com/ably/specification/pull/200/files#r1765476610) // @specPartial CHA-RL1h3 - Have tested the failure of the operation and the error that’s thrown. Have not yet implemented the "enter the recovery loop" (TODO: https://github.com/ably-labs/ably-chat-swift/issues/50) @@ -511,22 +528,29 @@ struct RoomLifecycleManagerTests { } } - // @specPartial CHA-RL2e - Haven’t implemented the part that refers to "transient disconnect timeouts"; TODO do this (https://github.com/ably-labs/ably-chat-swift/issues/48) + // @spec CHA-RL2e @Test func detach_transitionsToDetaching() async throws { // Given: A RoomLifecycleManager, with a contributor on whom calling `detach()` will not complete until after the "Then" part of this test (the motivation for this is to suppress the room from transitioning to DETACHED, so that we can assert its current state as being DETACHING) let contributorDetachOperation = SignallableChannelOperation() - let manager = await createManager(contributors: [createContributor(detachBehavior: contributorDetachOperation.behavior)]) + let contributor = createContributor(detachBehavior: contributorDetachOperation.behavior) + + let manager = await createManager( + // We set a transient disconnect timeout, just so we can check that it gets cleared, as the spec point specifies + forTestingWhatHappensWhenHasTransientDisconnectTimeoutForTheseContributorIDs: [contributor.id], + contributors: [contributor] + ) let statusChangeSubscription = await manager.onChange(bufferingPolicy: .unbounded) async let statusChange = statusChangeSubscription.first { _ in true } // When: `performDetachOperation()` is called on the lifecycle manager async let _ = try await manager.performDetachOperation() - // Then: It emits a status change to DETACHING, and its current state is DETACHING + // Then: It emits a status change to DETACHING, its current state is DETACHING, and it clears transient disconnect timeouts #expect(try #require(await statusChange).current == .detaching) #expect(await manager.current == .detaching) + #expect(await !manager.testsOnly_hasTransientDisconnectTimeoutForAnyContributor) // Post-test: Now that we’ve seen the DETACHING state, allow the contributor `detach` call to complete contributorDetachOperation.complete(result: .success) @@ -720,22 +744,29 @@ struct RoomLifecycleManagerTests { #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) + // @specPartial CHA-RL3c - Marked as specPartial due to spec point duplication (see comment on above test) @Test func release_transitionsToReleasing() async throws { // Given: A RoomLifecycleManager, with a contributor on whom calling `detach()` will not complete until after the "Then" part of this test (the motivation for this is to suppress the room from transitioning to RELEASED, so that we can assert its current state as being RELEASING) let contributorDetachOperation = SignallableChannelOperation() - let manager = await createManager(contributors: [createContributor(detachBehavior: contributorDetachOperation.behavior)]) + let contributor = createContributor(detachBehavior: contributorDetachOperation.behavior) + + let manager = await createManager( + // We set a transient disconnect timeout, just so we can check that it gets cleared, as the spec point specifies + forTestingWhatHappensWhenHasTransientDisconnectTimeoutForTheseContributorIDs: [contributor.id], + contributors: [contributor] + ) let statusChangeSubscription = await manager.onChange(bufferingPolicy: .unbounded) async let statusChange = statusChangeSubscription.first { _ in true } // When: `performReleaseOperation()` is called on the lifecycle manager async let _ = await manager.performReleaseOperation() - // Then: It emits a status change to RELEASING, and its current state is RELEASING + // Then: It emits a status change to RELEASING, its current state is RELEASING, and it clears transient disconnect timeouts #expect(try #require(await statusChange).current == .releasing) #expect(await manager.current == .releasing) + #expect(await !manager.testsOnly_hasTransientDisconnectTimeoutForAnyContributor) // Post-test: Now that we’ve seen the RELEASING state, allow the contributor `detach` call to complete contributorDetachOperation.complete(result: .success) @@ -959,7 +990,7 @@ struct RoomLifecycleManagerTests { #expect(pendingDiscontinuityEvent === contributorStateChange.reason) } - // @specPartial CHA-RL4b5 - Haven’t implemented the part that refers to "transient disconnect timeouts"; TODO do this (https://github.com/ably-labs/ably-chat-swift/issues/48) + // @spec CHA-RL4b5 @Test func contributorFailedEvent_withNoOperationInProgress() async throws { // Given: A RoomLifecycleManager, with no room lifecycle operation in progress @@ -967,9 +998,15 @@ struct RoomLifecycleManagerTests { // TODO: The .success is currently arbitrary since the spec doesn’t say what to do if detach fails (have asked in https://github.com/ably/specification/pull/200#discussion_r1777471810) createContributor(detachBehavior: .success), createContributor(detachBehavior: .success), + createContributor(detachBehavior: .success), ] let manager = await createManager( forTestingWhatHappensWhenCurrentlyIn: .initialized, // case arbitrary, just care that no operation is in progress + forTestingWhatHappensWhenHasTransientDisconnectTimeoutForTheseContributorIDs: [ + // Give 2 of the 3 contributors a transient disconnect timeout, so we can test that _all_ such timeouts get cleared (as the spec point specifies), not just those for the FAILED contributor + contributors[0].id, + contributors[1].id, + ], contributors: contributors ) @@ -992,12 +1029,15 @@ struct RoomLifecycleManagerTests { // Then: // - the room status transitions to failed, with the error of the status change being the `reason` of the contributor FAILED event // - and it calls `detach` on all contributors + // - it clears all transient disconnect timeouts _ = try #require(await failedStatusChange) #expect(await manager.current.isFailed) for contributor in contributors { #expect(await contributor.channel.detachCallCount == 1) } + + #expect(await !manager.testsOnly_hasTransientDisconnectTimeoutForAnyContributor) } // @spec CHA-RL4b6 @@ -1081,6 +1121,44 @@ struct RoomLifecycleManagerTests { #expect(await !manager.testsOnly_hasTransientDisconnectTimeout(for: contributor)) } + // @specPartial CHA-RL4b7 - This is marked as specPartial because at time of writing the spec point CHA-RL4b7 has been accidentally duplicated to specify two separate behaviours. This test is for the second of those two. TODO: change this one to @spec once spec fixed (see discussion in https://github.com/ably/specification/pull/200#discussion_r1763770348) + @Test + func contributorAttachedEvent_withNoOperationInProgress_clearsTransientDisconnectTimeouts() async throws { + // Given: A RoomLifecycleManager, with no room lifecycle operation in progress + let contributorThatWillEmitAttachedStateChange = createContributor() + let contributors = [ + contributorThatWillEmitAttachedStateChange, + createContributor(), + createContributor(), + ] + let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .initialized, // case arbitrary, just care that no operation is in progress + forTestingWhatHappensWhenHasTransientDisconnectTimeoutForTheseContributorIDs: [ + // Give 2 of the 3 contributors a transient disconnect timeout, so we can test that only the timeout for the ATTACHED contributor gets cleared, not all of them + contributorThatWillEmitAttachedStateChange.id, + contributors[1].id, + ], + contributors: contributors + ) + + // When: A contributor emits a state change to ATTACHED + let contributorAttachedStateChange = ARTChannelStateChange( + current: .attached, + previous: .attaching, // arbitrary + event: .attached, + reason: nil // arbitrary + ) + + await waitForManager(manager, toHandleContributorStateChange: contributorAttachedStateChange) { + await contributorThatWillEmitAttachedStateChange.channel.emitStateChange(contributorAttachedStateChange) + } + + // Then: The manager clears any transient disconnect timeout for that contributor + #expect(await !manager.testsOnly_hasTransientDisconnectTimeout(for: contributorThatWillEmitAttachedStateChange)) + // check the timeout for the other contributors didn’t get cleared + #expect(await manager.testsOnly_hasTransientDisconnectTimeout(for: contributors[1])) + } + // @specOneOf(1/2) CHA-RL4b8 @Test func contributorAttachedEvent_withNoOperationInProgress_roomNotAttached_allContributorsAttached() async throws { @@ -1146,14 +1224,24 @@ struct RoomLifecycleManagerTests { #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) + // @specPartial CHA-RL4b9 - Haven’t implemented "the room enters the RETRY loop"; TODO do this (https://github.com/ably-labs/ably-chat-swift/issues/51) @Test func contributorSuspendedEvent_withNoOperationInProgress() async throws { // Given: A RoomLifecycleManager with no lifecycle operation in progress - let contributor = createContributor() + let contributorThatWillEmitStateChange = createContributor() + let contributors = [ + contributorThatWillEmitStateChange, + createContributor(), + createContributor(), + ] let manager = await createManager( forTestingWhatHappensWhenCurrentlyIn: .initialized, // case arbitrary, just care that no operation is in progress - contributors: [contributor] + // Give 2 of the 3 contributors a transient disconnect timeout, so we can test that _all_ such timeouts get cleared (as the spec point specifies), not just those for the SUSPENDED contributor + forTestingWhatHappensWhenHasTransientDisconnectTimeoutForTheseContributorIDs: [ + contributorThatWillEmitStateChange.id, + contributors[1].id, + ], + contributors: [contributorThatWillEmitStateChange] ) let roomStatusSubscription = await manager.onChange(bufferingPolicy: .unbounded) @@ -1169,12 +1257,18 @@ struct RoomLifecycleManagerTests { resumed: false // arbitrary ) - await contributor.channel.emitStateChange(contributorStateChange) + await waitForManager(manager, toHandleContributorStateChange: contributorStateChange) { + await contributorThatWillEmitStateChange.channel.emitStateChange(contributorStateChange) + } - // Then: The room transitions to SUSPENDED, and this state change has error equal to the contributor state change’s `reason` + // Then: + // - The room transitions to SUSPENDED, and this state change has error equal to the contributor state change’s `reason` + // - All transient disconnect timeouts are cancelled let suspendedRoomStatusChange = try #require(await maybeSuspendedRoomStatusChange) #expect(suspendedRoomStatusChange.error === contributorStateChangeReason) #expect(await manager.current == .suspended(error: contributorStateChangeReason)) + + #expect(await !manager.testsOnly_hasTransientDisconnectTimeoutForAnyContributor) } }