Skip to content

Commit

Permalink
Implement CHA-RL9
Browse files Browse the repository at this point in the history 
Resolves #131.
  • Loading branch information
@lawrence-forooghian
lawrence-forooghian committed Nov 25, 2024
1 parent 4dd7207 commit 0418956
Show file tree
Hide file tree
Showing 5 changed files with 103 additions and 39 deletions.
24 changes: 18 additions & 6 deletions Sources/AblyChat/Errors.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ public enum ErrorCode: Int {
case roomIsReleasing = 102_102
case roomIsReleased = 102_103

case roomInInvalidState = 102_107

/// 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
Expand All @@ -49,12 +51,15 @@ public enum ErrorCode: Int {
.roomIsReleasing,
.roomIsReleased:
400
case .messagesAttachmentFailed,
.presenceAttachmentFailed,
.reactionsAttachmentFailed,
.occupancyAttachmentFailed,
.typingAttachmentFailed:
// TODO: This is 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.
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,
// CHA-RL9c
.roomInInvalidState:
500
}
}
Expand All @@ -74,6 +79,7 @@ internal enum ChatError {
case roomIsReleased
case presenceOperationRequiresRoomAttach(feature: RoomFeature)
case presenceOperationDisallowedForCurrentRoomStatus(feature: RoomFeature)
case roomInInvalidState(cause: ARTErrorInfo?)

/// The ``ARTErrorInfo.code`` that should be returned for this error.
internal var code: ErrorCode {
Expand Down Expand Up @@ -112,6 +118,8 @@ internal enum ChatError {
.roomIsReleasing
case .roomIsReleased:
.roomIsReleased
case .roomInInvalidState:
.roomInInvalidState
case .presenceOperationRequiresRoomAttach,
.presenceOperationDisallowedForCurrentRoomStatus:
.nonspecific
Expand Down Expand Up @@ -172,6 +180,8 @@ internal enum ChatError {
"To perform this \(Self.descriptionOfFeature(feature)) operation, you must first attach the room."
case let .presenceOperationDisallowedForCurrentRoomStatus(feature):
"This \(Self.descriptionOfFeature(feature)) operation can not be performed given the current room status."
case .roomInInvalidState:
"The room operation failed because the room was in an invalid state."
}
}

Expand All @@ -182,6 +192,8 @@ internal enum ChatError {
underlyingError
case let .detachmentFailed(_, underlyingError):
underlyingError
case let .roomInInvalidState(cause):
cause
case .inconsistentRoomOptions,
.roomInFailedState,
.roomIsReleasing,
Expand Down
2 changes: 1 addition & 1 deletion Sources/AblyChat/RoomFeature.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ internal protocol FeatureChannel: Sendable, EmitsDiscontinuities {
///
/// Implements the checks described by CHA-PR3d, CHA-PR3e, CHA-PR3f, and CHA-PR3g (and similar ones described by other functionality that performs contributor presence operations). Namely:
///
/// - CHA-PR3d, CHA-PR10d, CHA-PR6c, CHA-T2c: If the room is in the ATTACHING status, it waits for the current ATTACH to complete and then returns. If the current ATTACH fails, then it re-throws that operation’s error.
/// - CHA-RL9, which is invoked by CHA-PR3d, CHA-PR10d, CHA-PR6c, CHA-T2c: If the room is in the ATTACHING status, it waits for the next room status change. If the new status is ATTACHED, it returns. Else, it throws an `ARTErrorInfo` derived from ``ChatError.roomInInvalidState(cause:)``.
/// - CHA-PR3e, CHA-PR11e, CHA-PR6d, CHA-T2d: If the room is in the ATTACHED status, it returns immediately.
/// - CHA-PR3f, CHA-PR11f, CHA-PR6e, CHA-T2e: If the room is in the DETACHED status, it throws an `ARTErrorInfo` derived from ``ChatError.presenceOperationRequiresRoomAttach(feature:)``.
/// - // CHA-PR3g, CHA-PR11g, CHA-PR6f, CHA-T2f: If the room is in any other status, it throws an `ARTErrorInfo` derived from ``ChatError.presenceOperationDisallowedForCurrentRoomStatus(feature:)``.
Expand Down
65 changes: 47 additions & 18 deletions Sources/AblyChat/RoomLifecycleManager.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1192,31 +1192,60 @@ internal actor DefaultRoomLifecycleManager<Contributor: RoomLifecycleContributor
// MARK: - Waiting to be able to perform presence operations

internal func waitToBeAbleToPerformPresenceOperations(requestedByFeature requester: RoomFeature) async throws(ARTErrorInfo) {
switch status {
case let .attachingDueToAttachOperation(attachOperationID):
// CHA-PR3d, CHA-PR10d, CHA-PR6c, CHA-T2c
try await waitForCompletionOfOperationWithID(attachOperationID, requester: .waitToBeAbleToPerformPresenceOperations)
case .attachingDueToContributorStateChange:
// TODO: Spec doesn’t say what to do in this situation; asked in https://github.com/ably/specification/issues/228
fatalError("waitToBeAbleToPerformPresenceOperations doesn’t currently handle attachingDueToContributorStateChange")
case .attachingDueToRetryOperation:
// TODO: Spec doesn’t say what to do in this situation; asked in https://github.com/ably/specification/issues/228
fatalError("waitToBeAbleToPerformPresenceOperations doesn’t currently handle attachingDueToRetryOperation")
// Although this method’s implementation only uses the manager’s public
// API, it’s implemented as a method on the manager itself, so that the
// implementation is isolated to the manager and hence doesn’t “miss”
// any status changes. (There may be other ways to achieve the same
// effect; can revisit.)

switch status.toRoomStatus {
case .attaching:
// CHA-RL9, which is invoked by CHA-PR3d, CHA-PR10d, CHA-PR6c, CHA-T2c

// CHA-RL9a
let subscription = onRoomStatusChange(bufferingPolicy: .unbounded)
logger.log(message: "waitToBeAbleToPerformPresenceOperations waiting for status change", level: .debug)
#if DEBUG
for subscription in statusChangeWaitEventSubscriptions {
subscription.emit(.init())
}
#endif
let nextRoomStatusChange = await (subscription.first { _ in true })
logger.log(message: "waitToBeAbleToPerformPresenceOperations got status change \(String(describing: nextRoomStatusChange))", level: .debug)

// CHA-RL9b
// TODO: decide what to do if nextRoomStatusChange is nil; I believe that this will happen if the current Task is cancelled. For now, will just treat it as an invalid status change. Handle it properly in https://github.com/ably-labs/ably-chat-swift/issues/29
if nextRoomStatusChange?.current != .attached {
// CHA-RL9c
throw .init(chatError: .roomInInvalidState(cause: nextRoomStatusChange?.current.error))
}
case .attached:
// CHA-PR3e, CHA-PR11e, CHA-PR6d, CHA-T2d
break
case .detached, .detachedDueToRetryOperation:
case .detached:
// CHA-PR3f, CHA-PR11f, CHA-PR6e, CHA-T2e
throw .init(chatError: .presenceOperationRequiresRoomAttach(feature: requester))
case .detaching,
.failed,
.initialized,
.released,
.releasing,
.suspendedAwaitingStartOfRetryOperation,
.suspended:
default:
// CHA-PR3g, CHA-PR11g, CHA-PR6f, CHA-T2f
throw .init(chatError: .presenceOperationDisallowedForCurrentRoomStatus(feature: requester))
}
}

#if DEBUG
/// The manager emits a `StatusChangeWaitEvent` each time ``waitToBeAbleToPerformPresenceOperations(requestedByFeature:)`` is going to wait for a room status change. These events are emitted to support testing of the manager; see ``testsOnly_subscribeToStatusChangeWaitEvents``.
internal struct StatusChangeWaitEvent: Equatable {
// Nothing here currently, just created this type for consistency with OperationWaitEvent
}

// TODO: clean up old subscriptions (https://github.com/ably-labs/ably-chat-swift/issues/36)
/// Supports the ``testsOnly_subscribeToStatusChangeWaitEvents()`` method.
private var statusChangeWaitEventSubscriptions: [Subscription<StatusChangeWaitEvent>] = []

/// Returns a subscription which emits an event each time ``waitToBeAbleToPerformPresenceOperations(requestedByFeature:)`` is going to wait for a room status change.
internal func testsOnly_subscribeToStatusChangeWaitEvents() -> Subscription<StatusChangeWaitEvent> {
let subscription = Subscription<StatusChangeWaitEvent>(bufferingPolicy: .unbounded)
statusChangeWaitEventSubscriptions.append(subscription)
return subscription
}
#endif
}
18 changes: 18 additions & 0 deletions Sources/AblyChat/RoomStatus.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,24 @@ public enum RoomStatus: Sendable, Equatable {
case releasing
case released

internal var error: ARTErrorInfo? {
switch self {
case let .attaching(error):
error
case let .suspended(error):
error
case let .failed(error):
error
case .initialized,
.attached,
.detaching,
.detached,
.releasing,
.released:
nil
}
}

// Helpers to allow us to test whether a `RoomStatus` value has a certain case, without caring about the associated value. These are useful for in contexts where we want to use a `Bool` to communicate a case. For example:
//
// 1. testing (e.g. `#expect(status.isFailed)`)
Expand Down
33 changes: 19 additions & 14 deletions Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1876,12 +1876,15 @@ struct DefaultRoomLifecycleManagerTests {

// MARK: - Waiting to be able to perform presence operations

// @specOneOf(1/2) CHA-RL9a
// @spec CHA-RL9b
//
// @specPartial CHA-PR3d - Tests the wait described in the spec point, but not that the feature actually performs this wait nor the side effect. TODO change this to a specOneOf once the feature is implemented
// @specPartial CHA-PR10d - Tests the wait described in the spec point, but not that the feature actually performs this wait nor the side effect. TODO change this to a specOneOf once the feature is implemented
// @specPartial CHA-PR6c - Tests the wait described in the spec point, but not that the feature actually performs this wait nor the side effect. TODO change this to a specOneOf once the feature is implemented
// @specPartial CHA-T2c - Tests the wait described in the spec point, but not that the feature actually performs this wait nor the side effect. TODO change this to a specOneOf once the feature is implemented
@Test
func waitToBeAbleToPerformPresenceOperations_whenAttaching_whenAttachSucceeds() async throws {
func waitToBeAbleToPerformPresenceOperations_whenAttaching_whenTransitionsToAttached() async throws {
// Given: A DefaultRoomLifecycleManager, with an ATTACH operation in progress and hence in the ATTACHING status
let contributorAttachOperation = SignallableChannelOperation()

Expand All @@ -1900,26 +1903,28 @@ struct DefaultRoomLifecycleManagerTests {
_ = await roomStatusSubscription.attachingElements().first { _ in true }

// When: `waitToBeAbleToPerformPresenceOperations(requestedByFeature:)` is called on the lifecycle manager
let operationWaitSubscription = await manager.testsOnly_subscribeToOperationWaitEvents()
let statusChangeWaitSubscription = await manager.testsOnly_subscribeToStatusChangeWaitEvents()
async let waitToBeAbleToPerformPresenceOperationsResult: Void = manager.waitToBeAbleToPerformPresenceOperations(requestedByFeature: .messages /* arbitrary */ )

// Then: The manager waits for the ATTACH operation to complete
let operationWaitEvent = try #require(await operationWaitSubscription.first { _ in true })
#expect(operationWaitEvent.waitedOperationID == attachOperationID)
// Then: The manager waits for its room status to change
_ = try #require(await statusChangeWaitSubscription.first { _ in true })

// and When: The ATTACH operation succeeds
// and When: The ATTACH operation succeeds, thus putting the room in the ATTACHED status
contributorAttachOperation.complete(behavior: .success)

// Then: The call to `waitToBeAbleToPerformPresenceOperations(requestedByFeature:)` succeeds
try await waitToBeAbleToPerformPresenceOperationsResult
}

// @specOneOf(2/2) CHA-RL9a
// @spec CHA-RL9c
//
// @specPartial CHA-PR3d - Tests the wait described in the spec point, but not that the feature actually performs this wait nor the side effect. TODO change this to a specOneOf once the feature is implemented
// @specPartial CHA-PR10d - Tests the wait described in the spec point, but not that the feature actually performs this wait nor the side effect. TODO change this to a specOneOf once the feature is implemented
// @specPartial CHA-PR6c - Tests the wait described in the spec point, but not that the feature actually performs this wait nor the side effect. TODO change this to a specOneOf once the feature is implemented
// @specPartial CHA-T2c - Tests the wait described in the spec point, but not that the feature actually performs this wait nor the side effect. TODO change this to a specOneOf once the feature is implemented
@Test
func waitToBeAbleToPerformPresenceOperations_whenAttaching_whenAttachFails() async throws {
func waitToBeAbleToPerformPresenceOperations_whenAttaching_whenTransitionsToNonAttachedStatus() async throws {
// Given: A DefaultRoomLifecycleManager, with an ATTACH operation in progress and hence in the ATTACHING status
let contributorAttachOperation = SignallableChannelOperation()

Expand All @@ -1938,26 +1943,26 @@ struct DefaultRoomLifecycleManagerTests {
_ = await roomStatusSubscription.attachingElements().first { _ in true }

// When: `waitToBeAbleToPerformPresenceOperations(requestedByFeature:)` is called on the lifecycle manager
let operationWaitSubscription = await manager.testsOnly_subscribeToOperationWaitEvents()
let statusChangeWaitSubscription = await manager.testsOnly_subscribeToStatusChangeWaitEvents()
async let waitToBeAbleToPerformPresenceOperationsResult: Void = manager.waitToBeAbleToPerformPresenceOperations(requestedByFeature: .messages /* arbitrary */ )

// Then: The manager waits for the ATTACH operation to complete
let operationWaitEvent = try #require(await operationWaitSubscription.first { _ in true })
#expect(operationWaitEvent.waitedOperationID == attachOperationID)
// Then: The manager waits for its room status to change
_ = try #require(await statusChangeWaitSubscription.first { _ in true })

// and When: The ATTACH operation fails
// and When: The ATTACH operation fails, thus putting the room in the FAILED status (i.e. a non-ATTACHED status)
let contributorAttachError = ARTErrorInfo.createUnknownError() // arbitrary
contributorAttachOperation.complete(behavior: .completeAndChangeState(.failure(contributorAttachError), newState: .failed))

// Then: The call to `waitToBeAbleToPerformPresenceOperations(requestedByFeature:)` fails with the same error
// Then: The call to `waitToBeAbleToPerformPresenceOperations(requestedByFeature:)` fails with a `roomInInvalidState` error, whose cause is the error associated with the room status change
var caughtError: Error?
do {
try await waitToBeAbleToPerformPresenceOperationsResult
} catch {
caughtError = error
}

#expect(isChatError(caughtError, withCode: .messagesAttachmentFailed, cause: contributorAttachError))
let expectedCause = ARTErrorInfo(chatError: .attachmentFailed(feature: .messages, underlyingError: contributorAttachError)) // using our knowledge of CHA-RL1h4
#expect(isChatError(caughtError, withCode: .roomInInvalidState, cause: expectedCause))
}

// @specPartial CHA-PR3e - Tests the wait described in the spec point, but not that the feature actually performs this wait nor the side effect. TODO change this to a specOneOf once the feature is implemented
Expand Down

0 comments on commit 0418956

Please sign in to comment.