-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Room lifecycle in progress flag #412
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
Coverage Report
File Coverage
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a private method to avoid code repetition
atomic(operation: () => Promise<void>, priority: number): Promise<void> {
return this._mtx.runExclusive(() => {
this._operationInProgress = true
return operation().finally(() =>
this._operationInProgress = false
)
}, priority);
}
So, attach and all other ops would be
attach(): Promise<void> {
this._logger.trace('RoomLifecycleManager.attach();');
return this.atomic(async () => {
We can in the future - though I'd prefer to keep this PR scoped to just the functionality fixes for now. |
Sure, I mean, the suggestion is related to the fix. In the future, if we have more atomic operations, we don't need to worry about setting/clearing |
There are places
The place I don't think it works is in the |
Sure, thanks for the info! |
2b998fd
to
8136f0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There's a lot of places where we don't set the operation in progress flag - e.g. after detach. This change fixes this.
The async-mutex library works as highest value = higher priority. This change reverses the priority in the lifecycle manager to fix this.
8136f0c
to
c0becec
Compare
Context
Closes #406
CHA-723
Description
We weren't properly setting and unsetting the room operation in progress flag (we were only setting it to false when attaching). This change rectifies this.
Also included is the fix for async-mutex operation precedence - as the order was the wrong way around.
Checklist
Testing Instructions (Optional)
N/A