Skip to content

Commit

Permalink
Allow discontinuities without an error
Browse files Browse the repository at this point in the history
Although the JS SDK allows users to be notified of a discontinuity
without an associated error, when I copied the public API across in
20e7f5f I decided that this was probably unintentional and that we could
do better (i.e. that surely every discontinuity has an error).

However, in [1] Andy has pointed out at least a couple of ways in you
might have a discontinuity without an error. Namely:

1. RTL12 does not guarantee that an UPDATE with resumed == false has an
   accompanying error

2. > we raise discontinuities if you explicitly detach a room and then
   > bring it back again

So, allow discontinuities without an associated error.

[1] ably/specification#239 (comment)
  • Loading branch information
lawrence-forooghian committed Nov 25, 2024
1 parent 9c87a98 commit 8f4b1f9
Show file tree
Hide file tree
Showing 12 changed files with 38 additions and 44 deletions.
2 changes: 1 addition & 1 deletion Sources/AblyChat/DefaultMessages.swift
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,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<ARTErrorInfo> {
internal func subscribeToDiscontinuities() async -> Subscription<ARTErrorInfo?> {
await featureChannel.subscribeToDiscontinuities()
}

Expand Down
2 changes: 1 addition & 1 deletion Sources/AblyChat/DefaultOccupancy.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<ARTErrorInfo> {
internal func subscribeToDiscontinuities() async -> Subscription<ARTErrorInfo?> {
await featureChannel.subscribeToDiscontinuities()
}
}
2 changes: 1 addition & 1 deletion Sources/AblyChat/DefaultPresence.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<ARTErrorInfo> {
internal func subscribeToDiscontinuities() async -> Subscription<ARTErrorInfo?> {
await featureChannel.subscribeToDiscontinuities()
}

Expand Down
8 changes: 4 additions & 4 deletions Sources/AblyChat/DefaultRoomLifecycleContributor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<ARTErrorInfo>] = []
private var discontinuitySubscriptions: [Subscription<ARTErrorInfo?>] = []

internal init(channel: DefaultRoomLifecycleContributorChannel, feature: RoomFeature) {
self.channel = channel
Expand All @@ -12,14 +12,14 @@ internal actor DefaultRoomLifecycleContributor: RoomLifecycleContributor, EmitsD

// MARK: - Discontinuities

internal func emitDiscontinuity(_ error: ARTErrorInfo) {
internal func emitDiscontinuity(_ error: ARTErrorInfo?) {
for subscription in discontinuitySubscriptions {
subscription.emit(error)
}
}

internal func subscribeToDiscontinuities() -> Subscription<ARTErrorInfo> {
let subscription = Subscription<ARTErrorInfo>(bufferingPolicy: .unbounded)
internal func subscribeToDiscontinuities() -> Subscription<ARTErrorInfo?> {
let subscription = Subscription<ARTErrorInfo?>(bufferingPolicy: .unbounded)
// TODO: clean up old subscriptions (https://github.com/ably-labs/ably-chat-swift/issues/36)
discontinuitySubscriptions.append(subscription)
return subscription
Expand Down
2 changes: 1 addition & 1 deletion Sources/AblyChat/DefaultRoomReactions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<ARTErrorInfo> {
internal func subscribeToDiscontinuities() async -> Subscription<ARTErrorInfo?> {
await featureChannel.subscribeToDiscontinuities()
}

Expand Down
2 changes: 1 addition & 1 deletion Sources/AblyChat/DefaultTyping.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<ARTErrorInfo> {
internal func subscribeToDiscontinuities() async -> Subscription<ARTErrorInfo?> {
await featureChannel.subscribeToDiscontinuities()
}

Expand Down
2 changes: 1 addition & 1 deletion Sources/AblyChat/EmitsDiscontinuities.swift
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import Ably

public protocol EmitsDiscontinuities {
func subscribeToDiscontinuities() async -> Subscription<ARTErrorInfo>
func subscribeToDiscontinuities() async -> Subscription<ARTErrorInfo?>
}
2 changes: 1 addition & 1 deletion Sources/AblyChat/RoomFeature.swift
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ internal struct DefaultFeatureChannel: FeatureChannel {
internal var contributor: DefaultRoomLifecycleContributor
internal var roomLifecycleManager: RoomLifecycleManager

internal func subscribeToDiscontinuities() async -> Subscription<ARTErrorInfo> {
internal func subscribeToDiscontinuities() async -> Subscription<ARTErrorInfo?> {
await contributor.subscribeToDiscontinuities()
}

Expand Down
30 changes: 12 additions & 18 deletions Sources/AblyChat/RoomLifecycleManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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(_ error: ARTErrorInfo?) async
}

internal protocol RoomLifecycleManager: Sendable {
Expand Down Expand Up @@ -111,7 +111,7 @@ internal actor DefaultRoomLifecycleManager<Contributor: RoomLifecycleContributor
#if DEBUG
internal init(
testsOnly_status status: Status? = nil,
testsOnly_pendingDiscontinuityEvents pendingDiscontinuityEvents: [Contributor.ID: [ARTErrorInfo]]? = nil,
testsOnly_pendingDiscontinuityEvents pendingDiscontinuityEvents: [Contributor.ID: [ARTErrorInfo?]]? = nil,
testsOnly_idsOfContributorsWithTransientDisconnectTimeout idsOfContributorsWithTransientDisconnectTimeout: Set<Contributor.ID>? = nil,
contributors: [Contributor],
logger: InternalLogger,
Expand All @@ -130,7 +130,7 @@ internal actor DefaultRoomLifecycleManager<Contributor: RoomLifecycleContributor

private init(
status: Status?,
pendingDiscontinuityEvents: [Contributor.ID: [ARTErrorInfo]]?,
pendingDiscontinuityEvents: [Contributor.ID: [ARTErrorInfo?]]?,
idsOfContributorsWithTransientDisconnectTimeout: Set<Contributor.ID>?,
contributors: [Contributor],
logger: InternalLogger,
Expand Down Expand Up @@ -263,7 +263,7 @@ internal actor DefaultRoomLifecycleManager<Contributor: RoomLifecycleContributor
}

// 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] = []
var pendingDiscontinuityEvents: [ARTErrorInfo?] = []
var transientDisconnectTimeout: TransientDisconnectTimeout?
/// Whether a state change to `ATTACHED` has already been observed for this contributor.
var hasBeenAttached: Bool
Expand All @@ -279,7 +279,7 @@ internal actor DefaultRoomLifecycleManager<Contributor: RoomLifecycleContributor

init(
contributors: [Contributor],
pendingDiscontinuityEvents: [Contributor.ID: [ARTErrorInfo]],
pendingDiscontinuityEvents: [Contributor.ID: [ARTErrorInfo?]],
idsOfContributorsWithTransientDisconnectTimeout: Set<Contributor.ID>
) {
storage = contributors.reduce(into: [:]) { result, contributor in
Expand Down Expand Up @@ -397,7 +397,7 @@ internal actor DefaultRoomLifecycleManager<Contributor: RoomLifecycleContributor
return subscription
}

internal func testsOnly_pendingDiscontinuityEvents(for contributor: Contributor) -> [ARTErrorInfo] {
internal func testsOnly_pendingDiscontinuityEvents(for contributor: Contributor) -> [ARTErrorInfo?] {
contributorAnnotations[contributor].pendingDiscontinuityEvents
}

Expand Down Expand Up @@ -441,19 +441,16 @@ internal actor DefaultRoomLifecycleManager<Contributor: RoomLifecycleContributor
break
}

guard let reason = stateChange.reason else {
// TODO: Decide the right thing to do here (https://github.com/ably-labs/ably-chat-swift/issues/74)
preconditionFailure("State change event with resumed == false should have a reason")
}
let reason = stateChange.reason

if hasOperationInProgress {
// CHA-RL4a3
logger.log(message: "Recording pending discontinuity event \(reason) for contributor \(contributor)", level: .info)
logger.log(message: "Recording pending discontinuity event \(String(describing: reason)) for contributor \(contributor)", level: .info)

contributorAnnotations[contributor].pendingDiscontinuityEvents.append(reason)
} else {
// CHA-RL4a4
logger.log(message: "Emitting discontinuity event \(reason) for contributor \(contributor)", level: .info)
logger.log(message: "Emitting discontinuity event \(String(describing: reason)) for contributor \(contributor)", level: .info)

await contributor.emitDiscontinuity(reason)
}
Expand All @@ -465,12 +462,9 @@ internal actor DefaultRoomLifecycleManager<Contributor: RoomLifecycleContributor
if !stateChange.resumed, hadAlreadyAttached {
// CHA-RL4b1

guard let reason = stateChange.reason else {
// TODO: Decide the right thing to do here (https://github.com/ably-labs/ably-chat-swift/issues/74)
preconditionFailure("Non-initial ATTACHED state change with resumed == false should have a reason")
}
let reason = stateChange.reason

logger.log(message: "Recording pending discontinuity event \(reason) for contributor \(contributor)", level: .info)
logger.log(message: "Recording pending discontinuity event \(String(describing: reason)) for contributor \(contributor)", level: .info)

contributorAnnotations[contributor].pendingDiscontinuityEvents.append(reason)
}
Expand Down Expand Up @@ -861,7 +855,7 @@ internal actor DefaultRoomLifecycleManager<Contributor: RoomLifecycleContributor
logger.log(message: "Emitting pending discontinuity events", level: .info)
for contributor in contributors {
for pendingDiscontinuityEvent in contributorAnnotations[contributor].pendingDiscontinuityEvents {
logger.log(message: "Emitting pending discontinuity event \(pendingDiscontinuityEvent) to contributor \(contributor)", level: .info)
logger.log(message: "Emitting pending discontinuity event \(String(describing: pendingDiscontinuityEvent)) to contributor \(contributor)", level: .info)
await contributor.emitDiscontinuity(pendingDiscontinuityEvent)
}
}
Expand Down
18 changes: 9 additions & 9 deletions Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ struct DefaultRoomLifecycleManagerTests {

private func createManager(
forTestingWhatHappensWhenCurrentlyIn status: DefaultRoomLifecycleManager<MockRoomLifecycleContributor>.Status? = nil,
forTestingWhatHappensWhenHasPendingDiscontinuityEvents pendingDiscontinuityEvents: [MockRoomLifecycleContributor.ID: [ARTErrorInfo]]? = nil,
forTestingWhatHappensWhenHasPendingDiscontinuityEvents pendingDiscontinuityEvents: [MockRoomLifecycleContributor.ID: [ARTErrorInfo?]]? = nil,
forTestingWhatHappensWhenHasTransientDisconnectTimeoutForTheseContributorIDs idsOfContributorsWithTransientDisconnectTimeout: Set<MockRoomLifecycleContributor.ID>? = nil,
contributors: [MockRoomLifecycleContributor] = [],
clock: SimpleClock = MockSimpleClock()
Expand Down Expand Up @@ -262,7 +262,7 @@ 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]] = [
let pendingDiscontinuityEvents: [MockRoomLifecycleContributor.ID: [ARTErrorInfo?]] = [
contributors[1].id: [.init(domain: "SomeDomain", code: 123) /* arbitrary */ ],
contributors[2].id: [.init(domain: "SomeDomain", code: 456) /* arbitrary */ ],
]
Expand Down Expand Up @@ -333,7 +333,7 @@ struct DefaultRoomLifecycleManagerTests {
current: .attached,
previous: .detached, // arbitrary
event: .attached,
reason: .createUnknownError() // Not related to this test, just to avoid a crash in CHA-RL4b1 handling of this state change
reason: nil // arbitrary
)
)
)
Expand Down Expand Up @@ -961,7 +961,7 @@ struct DefaultRoomLifecycleManagerTests {
current: .attached,
previous: .attaching, // arbitrary
event: .attached,
reason: .createUnknownError() // Not related to this test, just to avoid a crash in CHA-RL4b1 handling of this state change
reason: nil // arbitrary
)
)
),
Expand Down Expand Up @@ -1010,7 +1010,7 @@ struct DefaultRoomLifecycleManagerTests {
current: .attached,
previous: .attaching, // arbitrary
event: .attached,
reason: .createUnknownError() // Not related to this test, just to avoid a crash in CHA-RL4b1 handling of this state change
reason: nil // arbitrary
)
)
),
Expand Down Expand Up @@ -1203,7 +1203,7 @@ struct DefaultRoomLifecycleManagerTests {
current: .attached,
previous: .attaching, // arbitrary
event: .attached,
reason: .createUnknownError() // Not related to this test, just to avoid a crash in CHA-RL4b1 handling of this state change
reason: nil // arbitrary
)

return .addSubscriptionAndEmitStateChange(contributorAttachedStateChange)
Expand Down Expand Up @@ -1248,7 +1248,7 @@ struct DefaultRoomLifecycleManagerTests {
current: .attached,
previous: .attaching, // arbitrary
event: .attached,
reason: .createUnknownError() // Not related to this test, just to avoid a crash in CHA-RL4b1 handling of this state change
reason: nil // arbitrary
)
) // Not related to this test, just so that the CHA-RL5d wait completes
),
Expand Down Expand Up @@ -1296,7 +1296,7 @@ struct DefaultRoomLifecycleManagerTests {
current: .attached,
previous: .attaching, // arbitrary
event: .attached,
reason: .createUnknownError() // Not related to this test, just to avoid a crash in CHA-RL4b1 handling of this state change
reason: nil // arbitrary
)
) // Not related to this test, just so that the CHA-RL5d wait completes
),
Expand Down Expand Up @@ -1794,7 +1794,7 @@ struct DefaultRoomLifecycleManagerTests {
current: .attached,
previous: .detached, // arbitrary
event: .attached,
reason: .createUnknownError() // Not related to this test, just to avoid a crash in CHA-RL4b1 handling of this state change
reason: nil // arbitrary
)
)
)
Expand Down
8 changes: 4 additions & 4 deletions Tests/AblyChatTests/Mocks/MockFeatureChannel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<ARTErrorInfo>] = []
private var discontinuitySubscriptions: [Subscription<ARTErrorInfo?>] = []
private let resultOfWaitToBeAbleToPerformPresenceOperations: Result<Void, ARTErrorInfo>?

init(
Expand All @@ -15,13 +15,13 @@ final actor MockFeatureChannel: FeatureChannel {
resultOfWaitToBeAbleToPerformPresenceOperations = resultOfWaitToBeAblePerformPresenceOperations
}

func subscribeToDiscontinuities() async -> Subscription<ARTErrorInfo> {
let subscription = Subscription<ARTErrorInfo>(bufferingPolicy: .unbounded)
func subscribeToDiscontinuities() async -> Subscription<ARTErrorInfo?> {
let subscription = Subscription<ARTErrorInfo?>(bufferingPolicy: .unbounded)
discontinuitySubscriptions.append(subscription)
return subscription
}

func emitDiscontinuity(_ discontinuity: ARTErrorInfo) {
func emitDiscontinuity(_ discontinuity: ARTErrorInfo?) {
for subscription in discontinuitySubscriptions {
subscription.emit(discontinuity)
}
Expand Down
4 changes: 2 additions & 2 deletions Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ actor MockRoomLifecycleContributor: RoomLifecycleContributor {
nonisolated let feature: RoomFeature
nonisolated let channel: MockRoomLifecycleContributorChannel

private(set) var emitDiscontinuityArguments: [ARTErrorInfo] = []
private(set) var emitDiscontinuityArguments: [ARTErrorInfo?] = []

init(feature: RoomFeature, channel: MockRoomLifecycleContributorChannel) {
self.feature = feature
self.channel = channel
}

func emitDiscontinuity(_ error: ARTErrorInfo) async {
func emitDiscontinuity(_ error: ARTErrorInfo?) async {
emitDiscontinuityArguments.append(error)
}
}

0 comments on commit 8f4b1f9

Please sign in to comment.