From 4af14ba83f81a8fc9b0046e50350e4912c491862 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 27 Nov 2024 10:45:24 -0300 Subject: [PATCH 01/14] Remove TODO for something now clarified in spec --- Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift b/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift index 0af1a8e0..8acfc9cc 100644 --- a/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift +++ b/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift @@ -276,7 +276,7 @@ struct DefaultRoomLifecycleManagerTests { // Then: It: // - emits all pending discontinuities to its contributors - // - clears all pending discontinuity events (TODO: I assume this is the intended behaviour, but confirm; have asked in https://github.com/ably/specification/pull/200/files#r1781917231) + // - clears all pending discontinuity events for contributor in contributors { let expectedPendingDiscontinuityEvents = pendingDiscontinuityEvents[contributor.id] ?? [] let emitDiscontinuityArguments = await contributor.emitDiscontinuityArguments From 8af2e846f901b8ebb19c2c916ed64207e56ffeb3 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 27 Nov 2024 10:47:38 -0300 Subject: [PATCH 02/14] Remove references to CHA-RL1h1 This spec point was removed because it was redundant. --- Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift | 2 -- 1 file changed, 2 deletions(-) diff --git a/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift b/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift index 8acfc9cc..afdf6ab5 100644 --- a/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift +++ b/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift @@ -309,7 +309,6 @@ struct DefaultRoomLifecycleManagerTests { } // @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) // @spec CHA-RL1h3 @Test func attach_whenContributorFailsToAttachAndEntersSuspended_transitionsToSuspendedAndPerformsRetryOperation() async throws { @@ -412,7 +411,6 @@ struct DefaultRoomLifecycleManagerTests { _ = try #require(await roomStatusChangeSubscription.first { $0.current == .attached }) // Room status changes to ATTACHED } - // @specOneOf(2/2) CHA-RL1h1 - tests that an error gets thrown when channel attach fails due to entering FAILED (TODO: but I don’t yet fully understand the meaning of CHA-RL1h1; outstanding question https://github.com/ably/specification/pull/200/files#r1765476610)) // @spec CHA-RL1h4 @Test func attach_whenContributorFailsToAttachAndEntersFailed_transitionsToFailed() async throws { From bff7a31d7a60dd64f22e147bd81e5468074c0338 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 27 Nov 2024 11:05:12 -0300 Subject: [PATCH 03/14] Implement correct CHA-RL2h1 error This has been clarified in the spec. --- Sources/AblyChat/RoomLifecycleManager.swift | 7 +------ Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift | 3 +-- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index 472d1fb9..f72ba1ad 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -956,12 +956,7 @@ internal actor DefaultRoomLifecycleManager 0) } From ce633cb39721a8a9d75e895b8d111a21b85858e5 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 27 Nov 2024 11:15:18 -0300 Subject: [PATCH 04/14] Fix comment Mistake in b6c3ddb. --- Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift b/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift index 65fe9c45..4a67f672 100644 --- a/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift +++ b/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift @@ -1352,7 +1352,7 @@ struct DefaultRoomLifecycleManagerTests { await contributor.channel.emitStateChange(contributorStateChange) } - // Then: The manager does not record a pending discontinuity event for this contributor, nor does it call `emitDiscontinuity` on the contributor (this is my interpretation of "no action should be taken" in CHA-RL4a1; i.e. that the actions described in CHA-RL4a2 and CHA-RL4a3 shouldn’t happen) (TODO: get clarification; have asked in https://github.com/ably/specification/pull/200#discussion_r1777385499) + // Then: The manager does not record a pending discontinuity event for this contributor, nor does it call `emitDiscontinuity` on the contributor (this is my interpretation of "no action should be taken" in CHA-RL4a1; i.e. that the actions described in CHA-RL4a3 and CHA-RL4a4 shouldn’t happen) (TODO: get clarification; have asked in https://github.com/ably/specification/pull/200#discussion_r1777385499) #expect(await manager.testsOnly_pendingDiscontinuityEvents(for: contributor).isEmpty) #expect(await contributor.emitDiscontinuityArguments.isEmpty) } From 62c017c0101c6269149c3bafef5786d601598f2b Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 27 Nov 2024 11:16:37 -0300 Subject: [PATCH 05/14] Remove TODO for something now clarified in spec --- Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift b/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift index 4a67f672..070e777a 100644 --- a/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift +++ b/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift @@ -1352,7 +1352,7 @@ struct DefaultRoomLifecycleManagerTests { await contributor.channel.emitStateChange(contributorStateChange) } - // Then: The manager does not record a pending discontinuity event for this contributor, nor does it call `emitDiscontinuity` on the contributor (this is my interpretation of "no action should be taken" in CHA-RL4a1; i.e. that the actions described in CHA-RL4a3 and CHA-RL4a4 shouldn’t happen) (TODO: get clarification; have asked in https://github.com/ably/specification/pull/200#discussion_r1777385499) + // Then: The manager does not record a pending discontinuity event for this contributor, nor does it call `emitDiscontinuity` on the contributor; this shows us that the actions described in CHA-RL4a3 and CHA-RL4a4 haven’t been performed #expect(await manager.testsOnly_pendingDiscontinuityEvents(for: contributor).isEmpty) #expect(await contributor.emitDiscontinuityArguments.isEmpty) } From 7d970af40058839de8887d1fc9ce0d6a67204f94 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 27 Nov 2024 11:35:31 -0300 Subject: [PATCH 06/14] Remove mention of CHA-RL4b6 It was a redundant spec point that was implied by CHA-RL4b7. --- 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 070e777a..a9f47092 100644 --- a/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift +++ b/Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift @@ -1549,7 +1549,7 @@ struct DefaultRoomLifecycleManagerTests { #expect(await !manager.testsOnly_hasTransientDisconnectTimeoutForAnyContributor) } - // @spec CHA-RL4b6 + // @specOneOf(1/2) CHA-RL4b7 - Tests that when a transient disconnect timeout already exists, a new one is not created func contributorAttachingEvent_withNoOperationInProgress_withTransientDisconnectTimeout() async throws { // Given: A DefaultRoomLifecycleManager, with no operation in progress, with a transient disconnect timeout for the contributor mentioned in "When:" let contributor = createContributor() @@ -1573,11 +1573,11 @@ struct DefaultRoomLifecycleManagerTests { await contributor.channel.emitStateChange(contributorStateChange) } - // Then: It does not set a new transient disconnect timeout (this is my interpretation of CHA-RL4b6’s “no action is needed”, i.e. that the spec point intends to just be the contrapositive of CHA-RL4b7) + // Then: It does not set a new transient disconnect timeout #expect(await manager.testsOnly_idOfTransientDisconnectTimeout(for: contributor) == idOfExistingTransientDisconnectTimeout) } - // @spec CHA-RL4b7 + // @specOneOf(2/2) CHA-RL4b7 - Tests that when the conditions of this spec point are fulfilled, a transient disconnect timeout is created @Test( arguments: [ nil, From 8713168a2e18715644528bc7085d737cce8efeba Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 27 Nov 2024 11:45:20 -0300 Subject: [PATCH 07/14] Fix comment Mistake in 775ec51. --- 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 f72ba1ad..8475deaf 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -386,7 +386,7 @@ internal actor DefaultRoomLifecycleManager Date: Wed, 27 Nov 2024 13:14:25 -0300 Subject: [PATCH 08/14] Introduce a DiscontinuityEvent type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is what you’ll now receive when you subscribe to a room feature’s discontinuities. I should have done this in 30cfd30, when we started allowing a discontinuity’s error to be nil; it’s not nice to work with an AsyncSequence that emits optional values, because it makes the return value of operators like `first` ambiguous; did you reach the end of the sequence or did you get a discontinuity without an associated error? --- .../AblyChatExample/Mocks/MockClients.swift | 10 +++---- Sources/AblyChat/DefaultMessages.swift | 2 +- Sources/AblyChat/DefaultOccupancy.swift | 2 +- Sources/AblyChat/DefaultPresence.swift | 2 +- .../DefaultRoomLifecycleContributor.swift | 10 +++---- Sources/AblyChat/DefaultRoomReactions.swift | 2 +- Sources/AblyChat/DefaultTyping.swift | 2 +- Sources/AblyChat/EmitsDiscontinuities.swift | 11 +++++++- Sources/AblyChat/RoomFeature.swift | 2 +- Sources/AblyChat/RoomLifecycleManager.swift | 28 ++++++++++--------- .../AblyChatTests/DefaultMessagesTests.swift | 4 +-- .../DefaultRoomLifecycleManagerTests.swift | 16 +++++------ .../DefaultRoomReactionsTests.swift | 4 +-- .../Mocks/MockFeatureChannel.swift | 8 +++--- .../Mocks/MockRoomLifecycleContributor.swift | 6 ++-- 15 files changed, 60 insertions(+), 49 deletions(-) diff --git a/Example/AblyChatExample/Mocks/MockClients.swift b/Example/AblyChatExample/Mocks/MockClients.swift index a328577b..831d7044 100644 --- a/Example/AblyChatExample/Mocks/MockClients.swift +++ b/Example/AblyChatExample/Mocks/MockClients.swift @@ -144,7 +144,7 @@ actor MockMessages: Messages { return message } - func subscribeToDiscontinuities() -> Subscription { + func subscribeToDiscontinuities() -> Subscription { fatalError("Not yet implemented") } } @@ -195,7 +195,7 @@ actor MockRoomReactions: RoomReactions { .init(mockAsyncSequence: createSubscription()) } - func subscribeToDiscontinuities() -> Subscription { + func subscribeToDiscontinuities() -> Subscription { fatalError("Not yet implemented") } } @@ -244,7 +244,7 @@ actor MockTyping: Typing { } } - func subscribeToDiscontinuities() -> Subscription { + func subscribeToDiscontinuities() -> Subscription { fatalError("Not yet implemented") } } @@ -348,7 +348,7 @@ actor MockPresence: Presence { .init(mockAsyncSequence: createSubscription()) } - func subscribeToDiscontinuities() -> Subscription { + func subscribeToDiscontinuities() -> Subscription { fatalError("Not yet implemented") } } @@ -383,7 +383,7 @@ actor MockOccupancy: Occupancy { OccupancyEvent(connections: 10, presenceMembers: 5) } - func subscribeToDiscontinuities() -> Subscription { + func subscribeToDiscontinuities() -> Subscription { fatalError("Not yet implemented") } } diff --git a/Sources/AblyChat/DefaultMessages.swift b/Sources/AblyChat/DefaultMessages.swift index ee83169f..5c2fea1b 100644 --- a/Sources/AblyChat/DefaultMessages.swift +++ b/Sources/AblyChat/DefaultMessages.swift @@ -109,7 +109,7 @@ internal final class DefaultMessages: Messages, EmitsDiscontinuities { } // (CHA-M7) Users may subscribe to discontinuity events to know when there’s been a break in messages that they need to resolve. Their listener will be called when a discontinuity event is triggered from the room lifecycle. - internal func subscribeToDiscontinuities() async -> Subscription { + internal func subscribeToDiscontinuities() async -> Subscription { await featureChannel.subscribeToDiscontinuities() } diff --git a/Sources/AblyChat/DefaultOccupancy.swift b/Sources/AblyChat/DefaultOccupancy.swift index 0f6db119..31c95dd3 100644 --- a/Sources/AblyChat/DefaultOccupancy.swift +++ b/Sources/AblyChat/DefaultOccupancy.swift @@ -50,7 +50,7 @@ internal final class DefaultOccupancy: Occupancy, EmitsDiscontinuities { } // (CHA-O5) Users may subscribe to discontinuity events to know when there’s been a break in occupancy. Their listener will be called when a discontinuity event is triggered from the room lifecycle. For occupancy, there shouldn’t need to be user action as most channels will send occupancy updates regularly as clients churn. - internal func subscribeToDiscontinuities() async -> Subscription { + internal func subscribeToDiscontinuities() async -> Subscription { await featureChannel.subscribeToDiscontinuities() } } diff --git a/Sources/AblyChat/DefaultPresence.swift b/Sources/AblyChat/DefaultPresence.swift index 038518fc..e3e9d96c 100644 --- a/Sources/AblyChat/DefaultPresence.swift +++ b/Sources/AblyChat/DefaultPresence.swift @@ -194,7 +194,7 @@ internal final class DefaultPresence: Presence, EmitsDiscontinuities { } // (CHA-PR8) Users may subscribe to discontinuity events to know when there’s been a break in presence. Their listener will be called when a discontinuity event is triggered from the room lifecycle. For presence, there shouldn’t need to be user action as the underlying core SDK will heal the presence set. - internal func subscribeToDiscontinuities() async -> Subscription { + internal func subscribeToDiscontinuities() async -> Subscription { await featureChannel.subscribeToDiscontinuities() } diff --git a/Sources/AblyChat/DefaultRoomLifecycleContributor.swift b/Sources/AblyChat/DefaultRoomLifecycleContributor.swift index 1671e003..d8fee69f 100644 --- a/Sources/AblyChat/DefaultRoomLifecycleContributor.swift +++ b/Sources/AblyChat/DefaultRoomLifecycleContributor.swift @@ -3,7 +3,7 @@ import Ably internal actor DefaultRoomLifecycleContributor: RoomLifecycleContributor, EmitsDiscontinuities, CustomDebugStringConvertible { internal nonisolated let channel: DefaultRoomLifecycleContributorChannel internal nonisolated let feature: RoomFeature - private var discontinuitySubscriptions: [Subscription] = [] + private var discontinuitySubscriptions: [Subscription] = [] internal init(channel: DefaultRoomLifecycleContributorChannel, feature: RoomFeature) { self.channel = channel @@ -12,14 +12,14 @@ internal actor DefaultRoomLifecycleContributor: RoomLifecycleContributor, EmitsD // MARK: - Discontinuities - internal func emitDiscontinuity(_ error: ARTErrorInfo?) { + internal func emitDiscontinuity(_ discontinuity: DiscontinuityEvent) { for subscription in discontinuitySubscriptions { - subscription.emit(error) + subscription.emit(discontinuity) } } - internal func subscribeToDiscontinuities() -> Subscription { - let subscription = Subscription(bufferingPolicy: .unbounded) + internal func subscribeToDiscontinuities() -> Subscription { + let subscription = Subscription(bufferingPolicy: .unbounded) // TODO: clean up old subscriptions (https://github.com/ably-labs/ably-chat-swift/issues/36) discontinuitySubscriptions.append(subscription) return subscription diff --git a/Sources/AblyChat/DefaultRoomReactions.swift b/Sources/AblyChat/DefaultRoomReactions.swift index b4b91017..1a670010 100644 --- a/Sources/AblyChat/DefaultRoomReactions.swift +++ b/Sources/AblyChat/DefaultRoomReactions.swift @@ -80,7 +80,7 @@ internal final class DefaultRoomReactions: RoomReactions, EmitsDiscontinuities { } // (CHA-ER5) Users may subscribe to discontinuity events to know when there’s been a break in reactions that they need to resolve. Their listener will be called when a discontinuity event is triggered from the room lifecycle. - internal func subscribeToDiscontinuities() async -> Subscription { + internal func subscribeToDiscontinuities() async -> Subscription { await featureChannel.subscribeToDiscontinuities() } diff --git a/Sources/AblyChat/DefaultTyping.swift b/Sources/AblyChat/DefaultTyping.swift index 2dbdfc02..0c41e917 100644 --- a/Sources/AblyChat/DefaultTyping.swift +++ b/Sources/AblyChat/DefaultTyping.swift @@ -160,7 +160,7 @@ internal final class DefaultTyping: Typing { } // (CHA-T7) Users may subscribe to discontinuity events to know when there’s been a break in typing indicators. Their listener will be called when a discontinuity event is triggered from the room lifecycle. For typing, there shouldn’t need to be user action as the underlying core SDK will heal the presence set. - internal func subscribeToDiscontinuities() async -> Subscription { + internal func subscribeToDiscontinuities() async -> Subscription { await featureChannel.subscribeToDiscontinuities() } diff --git a/Sources/AblyChat/EmitsDiscontinuities.swift b/Sources/AblyChat/EmitsDiscontinuities.swift index 1fbe2a1a..21b9e5ad 100644 --- a/Sources/AblyChat/EmitsDiscontinuities.swift +++ b/Sources/AblyChat/EmitsDiscontinuities.swift @@ -1,5 +1,14 @@ import Ably +public struct DiscontinuityEvent: Sendable, Equatable { + /// The error, if any, associated with this discontinuity. + public var error: ARTErrorInfo? + + public init(error: ARTErrorInfo? = nil) { + self.error = error + } +} + public protocol EmitsDiscontinuities { - func subscribeToDiscontinuities() async -> Subscription + func subscribeToDiscontinuities() async -> Subscription } diff --git a/Sources/AblyChat/RoomFeature.swift b/Sources/AblyChat/RoomFeature.swift index 4ee4ab65..5b9968f8 100644 --- a/Sources/AblyChat/RoomFeature.swift +++ b/Sources/AblyChat/RoomFeature.swift @@ -58,7 +58,7 @@ internal struct DefaultFeatureChannel: FeatureChannel { internal var contributor: DefaultRoomLifecycleContributor internal var roomLifecycleManager: RoomLifecycleManager - internal func subscribeToDiscontinuities() async -> Subscription { + internal func subscribeToDiscontinuities() async -> Subscription { await contributor.subscribeToDiscontinuities() } diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index 8475deaf..3db82709 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -37,7 +37,7 @@ internal protocol RoomLifecycleContributor: Identifiable, Sendable { /// Informs the contributor that there has been a break in channel continuity, which it should inform library users about. /// /// It is marked as `async` purely to make it easier to write mocks for this method (i.e. to use an actor as a mock). - func emitDiscontinuity(_ error: ARTErrorInfo?) async + func emitDiscontinuity(_ discontinuity: DiscontinuityEvent) async } internal protocol RoomLifecycleManager: Sendable { @@ -111,7 +111,7 @@ internal actor DefaultRoomLifecycleManager? = nil, contributors: [Contributor], logger: InternalLogger, @@ -130,7 +130,7 @@ internal actor DefaultRoomLifecycleManager?, contributors: [Contributor], logger: InternalLogger, @@ -263,7 +263,7 @@ internal actor DefaultRoomLifecycleManager ) { storage = contributors.reduce(into: [:]) { result, contributor in @@ -397,7 +397,7 @@ internal actor DefaultRoomLifecycleManager [ARTErrorInfo?] { + internal func testsOnly_pendingDiscontinuityEvents(for contributor: Contributor) -> [DiscontinuityEvent] { contributorAnnotations[contributor].pendingDiscontinuityEvents } @@ -445,14 +445,16 @@ internal actor DefaultRoomLifecycleManager.Status? = nil, - forTestingWhatHappensWhenHasPendingDiscontinuityEvents pendingDiscontinuityEvents: [MockRoomLifecycleContributor.ID: [ARTErrorInfo?]]? = nil, + forTestingWhatHappensWhenHasPendingDiscontinuityEvents pendingDiscontinuityEvents: [MockRoomLifecycleContributor.ID: [DiscontinuityEvent]]? = nil, forTestingWhatHappensWhenHasTransientDisconnectTimeoutForTheseContributorIDs idsOfContributorsWithTransientDisconnectTimeout: Set? = nil, contributors: [MockRoomLifecycleContributor] = [], clock: SimpleClock = MockSimpleClock() @@ -262,9 +262,9 @@ struct DefaultRoomLifecycleManagerTests { func attach_uponSuccess_emitsPendingDiscontinuityEvents() async throws { // Given: A DefaultRoomLifecycleManager, all of whose contributors’ calls to `attach` succeed let contributors = (1 ... 3).map { _ in createContributor(attachBehavior: .success) } - let pendingDiscontinuityEvents: [MockRoomLifecycleContributor.ID: [ARTErrorInfo?]] = [ - contributors[1].id: [.init(domain: "SomeDomain", code: 123) /* arbitrary */ ], - contributors[2].id: [.init(domain: "SomeDomain", code: 456) /* arbitrary */ ], + let pendingDiscontinuityEvents: [MockRoomLifecycleContributor.ID: [DiscontinuityEvent]] = [ + contributors[1].id: [.init(error: .init(domain: "SomeDomain", code: 123) /* arbitrary */ )], + contributors[2].id: [.init(error: .init(domain: "SomeDomain", code: 456) /* arbitrary */ )], ] let manager = await createManager( forTestingWhatHappensWhenHasPendingDiscontinuityEvents: pendingDiscontinuityEvents, @@ -282,7 +282,7 @@ struct DefaultRoomLifecycleManagerTests { let emitDiscontinuityArguments = await contributor.emitDiscontinuityArguments try #require(emitDiscontinuityArguments.count == expectedPendingDiscontinuityEvents.count) for (emitDiscontinuityArgument, expectedArgument) in zip(emitDiscontinuityArguments, expectedPendingDiscontinuityEvents) { - #expect(emitDiscontinuityArgument === expectedArgument) + #expect(emitDiscontinuityArgument == expectedArgument) } } @@ -1385,7 +1385,7 @@ struct DefaultRoomLifecycleManagerTests { try #require(pendingDiscontinuityEvents.count == 1) let pendingDiscontinuityEvent = pendingDiscontinuityEvents[0] - #expect(pendingDiscontinuityEvent === contributorStateChange.reason) + #expect(pendingDiscontinuityEvent.error === contributorStateChange.reason) } // @spec CHA-RL4a4 @@ -1416,7 +1416,7 @@ struct DefaultRoomLifecycleManagerTests { try #require(emitDiscontinuityArguments.count == 1) let discontinuity = emitDiscontinuityArguments[0] - #expect(discontinuity === contributorStateChange.reason) + #expect(discontinuity.error === contributorStateChange.reason) } // @specPartial CHA-RL4b1 - Tests the case where the contributor has been attached previously (TODO: I have changed the criteria for deciding whether an ATTACHED status change represents a discontinuity, to be based on whether there was a previous ATTACHED state change instead of whether the `attach()` call has completed; see https://github.com/ably/specification/issues/239 and change this back to specOneOf(1/2) once we’re aligned with spec again) @@ -1466,7 +1466,7 @@ struct DefaultRoomLifecycleManagerTests { try #require(pendingDiscontinuityEvents.count == 1) let pendingDiscontinuityEvent = pendingDiscontinuityEvents[0] - #expect(pendingDiscontinuityEvent === contributorStateChange.reason) + #expect(pendingDiscontinuityEvent.error === contributorStateChange.reason) // Teardown: Allow performDetachOperation() call to complete contributorDetachOperation.complete(behavior: .success) diff --git a/Tests/AblyChatTests/DefaultRoomReactionsTests.swift b/Tests/AblyChatTests/DefaultRoomReactionsTests.swift index c0940795..e9233750 100644 --- a/Tests/AblyChatTests/DefaultRoomReactionsTests.swift +++ b/Tests/AblyChatTests/DefaultRoomReactionsTests.swift @@ -71,12 +71,12 @@ struct DefaultRoomReactionsTests { let roomReactions = await DefaultRoomReactions(featureChannel: featureChannel, clientID: "mockClientId", roomID: "basketball", logger: TestLogger()) // When: The feature channel emits a discontinuity through `subscribeToDiscontinuities` - let featureChannelDiscontinuity = ARTErrorInfo.createUnknownError() // arbitrary + let featureChannelDiscontinuity = DiscontinuityEvent(error: ARTErrorInfo.createUnknownError() /* arbitrary */ ) let messagesDiscontinuitySubscription = await roomReactions.subscribeToDiscontinuities() await featureChannel.emitDiscontinuity(featureChannelDiscontinuity) // Then: The DefaultRoomReactions instance emits this discontinuity through `subscribeToDiscontinuities` let messagesDiscontinuity = try #require(await messagesDiscontinuitySubscription.first { _ in true }) - #expect(messagesDiscontinuity === featureChannelDiscontinuity) + #expect(messagesDiscontinuity == featureChannelDiscontinuity) } } diff --git a/Tests/AblyChatTests/Mocks/MockFeatureChannel.swift b/Tests/AblyChatTests/Mocks/MockFeatureChannel.swift index dde42044..63df47a4 100644 --- a/Tests/AblyChatTests/Mocks/MockFeatureChannel.swift +++ b/Tests/AblyChatTests/Mocks/MockFeatureChannel.swift @@ -4,7 +4,7 @@ import Ably final actor MockFeatureChannel: FeatureChannel { let channel: RealtimeChannelProtocol // TODO: clean up old subscriptions (https://github.com/ably-labs/ably-chat-swift/issues/36) - private var discontinuitySubscriptions: [Subscription] = [] + private var discontinuitySubscriptions: [Subscription] = [] private let resultOfWaitToBeAbleToPerformPresenceOperations: Result? init( @@ -15,13 +15,13 @@ final actor MockFeatureChannel: FeatureChannel { resultOfWaitToBeAbleToPerformPresenceOperations = resultOfWaitToBeAblePerformPresenceOperations } - func subscribeToDiscontinuities() async -> Subscription { - let subscription = Subscription(bufferingPolicy: .unbounded) + func subscribeToDiscontinuities() async -> Subscription { + let subscription = Subscription(bufferingPolicy: .unbounded) discontinuitySubscriptions.append(subscription) return subscription } - func emitDiscontinuity(_ discontinuity: ARTErrorInfo?) { + func emitDiscontinuity(_ discontinuity: DiscontinuityEvent) { for subscription in discontinuitySubscriptions { subscription.emit(discontinuity) } diff --git a/Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift b/Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift index eb6c5f48..e09bbbfc 100644 --- a/Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift +++ b/Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift @@ -5,14 +5,14 @@ actor MockRoomLifecycleContributor: RoomLifecycleContributor { nonisolated let feature: RoomFeature nonisolated let channel: MockRoomLifecycleContributorChannel - private(set) var emitDiscontinuityArguments: [ARTErrorInfo?] = [] + private(set) var emitDiscontinuityArguments: [DiscontinuityEvent] = [] init(feature: RoomFeature, channel: MockRoomLifecycleContributorChannel) { self.feature = feature self.channel = channel } - func emitDiscontinuity(_ error: ARTErrorInfo?) async { - emitDiscontinuityArguments.append(error) + func emitDiscontinuity(_ discontinuity: DiscontinuityEvent) async { + emitDiscontinuityArguments.append(discontinuity) } } From 5643cc69faf84a833465f7b15e8b84325a9af47d Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 27 Nov 2024 13:31:11 -0300 Subject: [PATCH 09/14] Allow a single pending discontinuity event per contributor This is implied by the latest version of CHA-RL4a3, which states that you must not overwrite an existing pending discontinuity (will implement and test this behaviour in the next commit). --- Sources/AblyChat/RoomLifecycleManager.swift | 25 +++++++-------- .../DefaultRoomLifecycleManagerTests.swift | 32 +++++++------------ 2 files changed, 24 insertions(+), 33 deletions(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index 3db82709..1023a298 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -111,7 +111,7 @@ internal actor DefaultRoomLifecycleManager? = nil, contributors: [Contributor], logger: InternalLogger, @@ -130,7 +130,7 @@ internal actor DefaultRoomLifecycleManager?, contributors: [Contributor], logger: InternalLogger, @@ -262,8 +262,7 @@ internal actor DefaultRoomLifecycleManager? } - // TODO: Not clear whether there can be multiple or just one (asked in https://github.com/ably/specification/pull/200/files#r1781927850) - var pendingDiscontinuityEvents: [DiscontinuityEvent] = [] + var pendingDiscontinuityEvent: DiscontinuityEvent? var transientDisconnectTimeout: TransientDisconnectTimeout? /// Whether a state change to `ATTACHED` has already been observed for this contributor. var hasBeenAttached: Bool @@ -279,12 +278,12 @@ internal actor DefaultRoomLifecycleManager ) { storage = contributors.reduce(into: [:]) { result, contributor in result[contributor.id] = .init( - pendingDiscontinuityEvents: pendingDiscontinuityEvents[contributor.id] ?? [], + pendingDiscontinuityEvent: pendingDiscontinuityEvents[contributor.id], transientDisconnectTimeout: idsOfContributorsWithTransientDisconnectTimeout.contains(contributor.id) ? .init() : nil, hasBeenAttached: false ) @@ -308,7 +307,7 @@ internal actor DefaultRoomLifecycleManager [DiscontinuityEvent] { - contributorAnnotations[contributor].pendingDiscontinuityEvents + internal func testsOnly_pendingDiscontinuityEvent(for contributor: Contributor) -> DiscontinuityEvent? { + contributorAnnotations[contributor].pendingDiscontinuityEvent } internal func testsOnly_hasTransientDisconnectTimeout(for contributor: Contributor) -> Bool { @@ -448,7 +447,7 @@ internal actor DefaultRoomLifecycleManager.Status? = nil, - forTestingWhatHappensWhenHasPendingDiscontinuityEvents pendingDiscontinuityEvents: [MockRoomLifecycleContributor.ID: [DiscontinuityEvent]]? = nil, + forTestingWhatHappensWhenHasPendingDiscontinuityEvents pendingDiscontinuityEvents: [MockRoomLifecycleContributor.ID: DiscontinuityEvent]? = nil, forTestingWhatHappensWhenHasTransientDisconnectTimeoutForTheseContributorIDs idsOfContributorsWithTransientDisconnectTimeout: Set? = nil, contributors: [MockRoomLifecycleContributor] = [], clock: SimpleClock = MockSimpleClock() @@ -262,9 +262,9 @@ struct DefaultRoomLifecycleManagerTests { func attach_uponSuccess_emitsPendingDiscontinuityEvents() async throws { // Given: A DefaultRoomLifecycleManager, all of whose contributors’ calls to `attach` succeed let contributors = (1 ... 3).map { _ in createContributor(attachBehavior: .success) } - let pendingDiscontinuityEvents: [MockRoomLifecycleContributor.ID: [DiscontinuityEvent]] = [ - contributors[1].id: [.init(error: .init(domain: "SomeDomain", code: 123) /* arbitrary */ )], - contributors[2].id: [.init(error: .init(domain: "SomeDomain", code: 456) /* arbitrary */ )], + let pendingDiscontinuityEvents: [MockRoomLifecycleContributor.ID: DiscontinuityEvent] = [ + contributors[1].id: .init(error: .init(domain: "SomeDomain", code: 123) /* arbitrary */ ), + contributors[2].id: .init(error: .init(domain: "SomeDomain", code: 456) /* arbitrary */ ), ] let manager = await createManager( forTestingWhatHappensWhenHasPendingDiscontinuityEvents: pendingDiscontinuityEvents, @@ -278,16 +278,14 @@ struct DefaultRoomLifecycleManagerTests { // - emits all pending discontinuities to its contributors // - clears all pending discontinuity events for contributor in contributors { - let expectedPendingDiscontinuityEvents = pendingDiscontinuityEvents[contributor.id] ?? [] + let expectedPendingDiscontinuityEvent = pendingDiscontinuityEvents[contributor.id] let emitDiscontinuityArguments = await contributor.emitDiscontinuityArguments - try #require(emitDiscontinuityArguments.count == expectedPendingDiscontinuityEvents.count) - for (emitDiscontinuityArgument, expectedArgument) in zip(emitDiscontinuityArguments, expectedPendingDiscontinuityEvents) { - #expect(emitDiscontinuityArgument == expectedArgument) - } + try #require(emitDiscontinuityArguments.count == (expectedPendingDiscontinuityEvent == nil ? 0 : 1)) + #expect(emitDiscontinuityArguments.first == expectedPendingDiscontinuityEvent) } for contributor in contributors { - #expect(await manager.testsOnly_pendingDiscontinuityEvents(for: contributor).isEmpty) + #expect(await manager.testsOnly_pendingDiscontinuityEvent(for: contributor) == nil) } } @@ -1353,7 +1351,7 @@ struct DefaultRoomLifecycleManagerTests { } // Then: The manager does not record a pending discontinuity event for this contributor, nor does it call `emitDiscontinuity` on the contributor; this shows us that the actions described in CHA-RL4a3 and CHA-RL4a4 haven’t been performed - #expect(await manager.testsOnly_pendingDiscontinuityEvents(for: contributor).isEmpty) + #expect(await manager.testsOnly_pendingDiscontinuityEvent(for: contributor) == nil) #expect(await contributor.emitDiscontinuityArguments.isEmpty) } @@ -1381,10 +1379,7 @@ struct DefaultRoomLifecycleManagerTests { } // Then: The manager records a pending discontinuity event for this contributor, and this discontinuity event has error equal to the contributor UPDATE event’s `reason` - let pendingDiscontinuityEvents = await manager.testsOnly_pendingDiscontinuityEvents(for: contributor) - try #require(pendingDiscontinuityEvents.count == 1) - - let pendingDiscontinuityEvent = pendingDiscontinuityEvents[0] + let pendingDiscontinuityEvent = try #require(await manager.testsOnly_pendingDiscontinuityEvent(for: contributor)) #expect(pendingDiscontinuityEvent.error === contributorStateChange.reason) } @@ -1462,10 +1457,7 @@ struct DefaultRoomLifecycleManagerTests { } // Then: The manager records a pending discontinuity event for this contributor, and this discontinuity event has error equal to the contributor ATTACHED event’s `reason` - let pendingDiscontinuityEvents = await manager.testsOnly_pendingDiscontinuityEvents(for: contributor) - try #require(pendingDiscontinuityEvents.count == 1) - - let pendingDiscontinuityEvent = pendingDiscontinuityEvents[0] + let pendingDiscontinuityEvent = try #require(await manager.testsOnly_pendingDiscontinuityEvent(for: contributor)) #expect(pendingDiscontinuityEvent.error === contributorStateChange.reason) // Teardown: Allow performDetachOperation() call to complete @@ -1496,7 +1488,7 @@ struct DefaultRoomLifecycleManagerTests { } // Then: The manager does not record a pending discontinuity event for this contributor - #expect(await manager.testsOnly_pendingDiscontinuityEvents(for: contributor).isEmpty) + #expect(await manager.testsOnly_pendingDiscontinuityEvent(for: contributor) == nil) } // @spec CHA-RL4b5 From d126bb8c23a7650c5975197890415dcd71482835 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 27 Nov 2024 14:02:59 -0300 Subject: [PATCH 10/14] Extract pending discontinuity recording into method Will add further behaviour here. --- Sources/AblyChat/RoomLifecycleManager.swift | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index 1023a298..690197b4 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -444,10 +444,7 @@ internal actor DefaultRoomLifecycleManager Date: Wed, 27 Nov 2024 13:54:32 -0300 Subject: [PATCH 11/14] Do not replace existing pending discontinuity event This requirement was added to CHA-RL4a3 subsequent to my implementing the spec point. --- Sources/AblyChat/RoomLifecycleManager.swift | 6 ++++ .../DefaultRoomLifecycleManagerTests.swift | 34 ++++++++++++++++++- 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index 690197b4..09b13f86 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -577,6 +577,12 @@ internal actor DefaultRoomLifecycleManager Date: Wed, 27 Nov 2024 14:37:44 -0300 Subject: [PATCH 12/14] Implement CHA-RL3j --- Sources/AblyChat/RoomLifecycleManager.swift | 7 +++- .../DefaultRoomLifecycleManagerTests.swift | 39 ++++++++++++++----- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index 09b13f86..d6bd1f6c 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -1039,8 +1039,11 @@ internal actor DefaultRoomLifecycleManager.Status] + ) + func release_whenDetachedOrInitialized(status: DefaultRoomLifecycleManager.Status) async throws { + // Given: A DefaultRoomLifecycleManager in the DETACHED or INITIALIZED status let contributor = createContributor() - let manager = await createManager(forTestingWhatHappensWhenCurrentlyIn: .detached, contributors: [contributor]) + let manager = await createManager(forTestingWhatHappensWhenCurrentlyIn: status, contributors: [contributor]) let statusChangeSubscription = await manager.onRoomStatusChange(bufferingPolicy: .unbounded) async let statusChange = statusChangeSubscription.first { _ in true } @@ -777,7 +783,10 @@ struct DefaultRoomLifecycleManagerTests { // This allows us to prolong the execution of the RELEASE triggered in (1) detachBehavior: contributorDetachOperation.behavior ) - let manager = await createManager(contributors: [contributor]) + let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .attached, // arbitrary non-{RELEASED, DETACHED, INITIALIZED} status, so that the first RELEASE gets as far as CHA-RL3l + contributors: [contributor] + ) let firstReleaseOperationID = UUID() let secondReleaseOperationID = UUID() @@ -823,6 +832,7 @@ struct DefaultRoomLifecycleManagerTests { let contributor = createContributor(detachBehavior: contributorDetachOperation.behavior) let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .attached, // arbitrary non-{RELEASED, DETACHED, INITIALIZED} status, so that we get as far as CHA-RL3l // 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] @@ -857,7 +867,10 @@ struct DefaultRoomLifecycleManagerTests { createContributor(initialState: .detached /* arbitrary non-FAILED */, detachBehavior: .success), ] - let manager = await createManager(contributors: contributors) + let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .attached, // arbitrary non-{RELEASED, DETACHED, INITIALIZED} status, so that we get as far as CHA-RL3l + contributors: contributors + ) let statusChangeSubscription = await manager.onRoomStatusChange(bufferingPolicy: .unbounded) async let releasedStatusChange = statusChangeSubscription.first { $0.current == .released } @@ -897,7 +910,11 @@ struct DefaultRoomLifecycleManagerTests { let clock = MockSimpleClock() - let manager = await createManager(contributors: [contributor], clock: clock) + let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .attached, // arbitrary non-{RELEASED, DETACHED, INITIALIZED} status, so that we get as far as CHA-RL3l + contributors: [contributor], + clock: clock + ) // Then: When `performReleaseOperation()` is called on the manager await manager.performReleaseOperation() @@ -917,7 +934,11 @@ struct DefaultRoomLifecycleManagerTests { let clock = MockSimpleClock() - let manager = await createManager(contributors: [contributor], clock: clock) + let manager = await createManager( + forTestingWhatHappensWhenCurrentlyIn: .attached, // arbitrary non-{RELEASED, DETACHED, INITIALIZED} status, so that we get as far as CHA-RL3l + contributors: [contributor], + clock: clock + ) let statusChangeSubscription = await manager.onRoomStatusChange(bufferingPolicy: .unbounded) async let releasedStatusChange = statusChangeSubscription.first { $0.current == .released } From c8286453e71cd8d375e51f58c06785fe97d67c48 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 27 Nov 2024 15:05:20 -0300 Subject: [PATCH 13/14] Align error status codes with spec at e3e1be2 --- Sources/AblyChat/Errors.swift | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/Sources/AblyChat/Errors.swift b/Sources/AblyChat/Errors.swift index fcf4fb38..c955e181 100644 --- a/Sources/AblyChat/Errors.swift +++ b/Sources/AblyChat/Errors.swift @@ -38,26 +38,25 @@ public enum ErrorCode: Int { /// The ``ARTErrorInfo.statusCode`` that should be returned for this error. internal var statusCode: Int { - // TODO: These are currently a guess, revisit once outstanding spec question re status codes is answered (https://github.com/ably/specification/pull/200#discussion_r1755222945), and also revisit in https://github.com/ably-labs/ably-chat-swift/issues/32 + // These status codes are taken from the "Chat-specific Error Codes" section of the spec. switch self { case .nonspecific, .inconsistentRoomOptions, - .messagesDetachmentFailed, - .presenceDetachmentFailed, - .reactionsDetachmentFailed, - .occupancyDetachmentFailed, - .typingDetachmentFailed, .roomInFailedState, .roomIsReleasing, .roomIsReleased: 400 case - // TODO: These *AttachmentFailed ones are currently a best guess based on the limited status code information given in the spec at time of writing (i.e. CHA-RL1h4); it's not clear to me whether these error codes are always meant to have the same status code. Revisit once aforementioned spec question re status codes answered. .messagesAttachmentFailed, .presenceAttachmentFailed, .reactionsAttachmentFailed, .occupancyAttachmentFailed, .typingAttachmentFailed, + .messagesDetachmentFailed, + .presenceDetachmentFailed, + .reactionsDetachmentFailed, + .occupancyDetachmentFailed, + .typingDetachmentFailed, // CHA-RL9c .roomInInvalidState: 500 From b41cadc5ecb6152a0d2e6111f8fbdebd6ea0e46b Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Wed, 27 Nov 2024 15:40:37 -0300 Subject: [PATCH 14/14] Implement CHA-RL4a2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Have used the modified “has been attached” criterion from 8b5aa47, for consistency. --- Sources/AblyChat/RoomLifecycleManager.swift | 5 + .../DefaultRoomLifecycleManagerTests.swift | 95 +++++++++++++++++-- 2 files changed, 93 insertions(+), 7 deletions(-) diff --git a/Sources/AblyChat/RoomLifecycleManager.swift b/Sources/AblyChat/RoomLifecycleManager.swift index d6bd1f6c..de05474e 100644 --- a/Sources/AblyChat/RoomLifecycleManager.swift +++ b/Sources/AblyChat/RoomLifecycleManager.swift @@ -440,6 +440,11 @@ internal actor DefaultRoomLifecycleManager