[ECO-5122] Implement the "asynchronously" part of CHA-RL1h5 #174
Conversation
|
Warning Rate limit exceeded@lawrence-forooghian has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce significant updates to the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Sources/AblyChat/RoomLifecycleManager.swift (1)
1238-1247: Add delay and cancellation check in infinite retry loopThe infinite
while trueloop retries detaching contributors without any delay or exit condition. This may lead to excessive resource usage. Consider adding a delay between retries and checking for task cancellation to improve performance and responsiveness.Apply this diff to add a delay and handle cancellation:
while true { do { logger.log(message: "Detaching non-failed contributor \(contributor)", level: .info) try await contributor.channel.detach() break } catch { logger.log(message: "Failed to detach non-failed contributor \(contributor), error \(error). Retrying.", level: .info) + // Add a small delay before retrying + try await clock.sleep(timeInterval: 0.25) + // Check for task cancellation + try Task.checkCancellation() } }Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1)
1388-1412: Consider adding delay and cancellation checks in retry logicIn the test
rundown_ifADetachFailsItIsRetriedUntilSuccess, the retry logic does not include any delay, potentially causing tight loops during testing.Apply this diff to simulate delay and enhance test realism:
let detachResult = { @Sendable (callCount: Int) async -> MockRoomLifecycleContributorChannel.AttachOrDetachBehavior in if callCount == 1 { return .failure(.create(withCode: 123, message: "")) } else { return .success } } + let sleepOperation = SignallableSleepOperation() + let clock = MockSimpleClock(sleepBehavior: sleepOperation.behavior) let contributors = [ createContributor( detachBehavior: .fromFunction(detachResult) ), ] + let manager = await createManager(contributors: contributors, clock: clock) // When: `performRundownOperation()` is called on the lifecycle manager await manager.performRundownOperation(errorForFailedStatus: .createUnknownError() /* arbitrary */ ) + // Allow the sleep to complete + sleepOperation.complete() // Then: the lifecycle manager calls `detach` twice on the contributor (i.e. it retries the failed detach) #expect(await contributors[0].channel.detachCallCount == 2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Sources/AblyChat/RoomLifecycleManager.swift(11 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(6 hunks)
🔇 Additional comments (2)
Sources/AblyChat/RoomLifecycleManager.swift (1)
854-862: Ensure proper error propagation and state transition
When scheduling the RUNDOWN operation after a contributor fails to attach, make sure that the error is propagated correctly and the state transitions are handled appropriately.
Run the following script to verify that all callers handle the thrown error correctly:
Ensure that all calls are properly using try and handling possible errors.
✅ Verification successful
Based on the search results, I can now provide a final response:
Error handling is properly implemented for performAttachOperation
The verification shows that:
performAttachOperation()is correctly marked asthrowsand is always called with proper error handling- All calls to this method in
Room.swiftand tests usetry await - The implementation properly propagates errors through the
_performAttachOperationandbodyOfAttachOperationmethods - The test suite includes comprehensive error handling scenarios, including:
- Room release state errors
- Attachment failures
- State transition errors
- Contributor failures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `performAttachOperation` and ensure errors are handled.
# Search for calls to `performAttachOperation` not within a `try` block.
rg -U 'performAttachOperation\(\)(?!\s*(try|await))' -A 5
Length of output: 333
Script:
#!/bin/bash
# Description: Find all calls to `performAttachOperation` with proper context
rg -U "performAttachOperation\(" -A 5
Length of output: 15880
Script:
#!/bin/bash
# Description: Use ast-grep to find function calls to performAttachOperation
ast-grep --pattern 'performAttachOperation()'
Length of output: 47
Script:
#!/bin/bash
# Description: Find the implementation of performAttachOperation to understand its error handling
rg -U "func performAttachOperation" -A 20
Length of output: 5976
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1)
1350-1384: New tests for RUNDOWN operation are comprehensive
The added tests effectively verify the behavior of the RUNDOWN operation, ensuring non-failed contributors are detached and retries happen correctly.
|
Tests are failing due to #169. |
lawrence-forooghian
force-pushed
the
152-implement-async-room-get-spec
branch
from
90f9c55 to
e7612c9
Compare
We don’t use this “will” language elsewhere.
This implementation reflects my suggested spec changes [1] which aim to preserve lifecycle operation atomicity by introducing a new RUNDOWN operation: > The `ATTACH` operation ends in CHA-RL1h4, implying that the CHA-RL1h5 > asynchronous detach happens _outside of any room lifecycle operation_. > This means that another room lifecycle operation could run at the same > time as this detach operation, which doesn't seem intentional. > > I looked at the JS implementation [2] and it seems that it keeps the > lifecycle manager’s mutex locked during this "rundown" (as it calls the > CHA-RL1h5 detach operation). But this is not implied in the spec. I > think that to translate this behaviour to the spec, which implements > mutual exclusion through lifecycle operations, we should do something > analogous to the `RETRY` operation; that is, define a new internal-only > room lifecycle operation (I’ll call it `RUNDOWN` for want of a better > term), which is scheduled by CHA-RL1h5 and which: > > - performs the detach-all-non-`FAILED`-contributors behaviour of CHA-RL1h5 > - implements the retry behaviour of CHA-RL1h6 Resolves #119. [1] ably/specification#253 [2] https://github.com/ably/ably-chat-js/blob/e8380583424a83f7151405cc0716e01302295eb6/src/core/room-lifecycle-manager.ts#L506-L509
lawrence-forooghian
force-pushed
the
119-CHA-RL1h5-asynchronously
branch
from
9a392b6 to
77d0791
Compare
|
What does this mean @lawrence-forooghian:
|
|
Yeah, that spec point isn't very clear; I hope that we can also make it clearer when we address ably/specification#253. My interpretation is that it means something like:
|
But they all |
What do you mean?
You mean because CHA-RL1h4 says that the |
It was re "in a manner that does not block", but is nonsense anyway, ignore.
So attach operation might complete (failed) before channels are detached? What state room is between it goes to FAILED and all channels are detached? |
Yep.
It remains |
Note: This PR is based on top of #171; please review that one first.
Note that CHA-RL1h5, as currently written, does not respect operation atomicity. I’ve raised ably/specification#253 for this, and my implementation implements the changes that I’ve suggested there, introducing a new
RUNDOWNroom lifecycle operation. See commit messages and code comments for more details.Resolves #119.
Summary by CodeRabbit
New Features
RUNDOWNoperation for improved room lifecycle management, allowing for better handling of failed contributors.Bug Fixes
Documentation