This repository has been archived by the owner on Dec 3, 2024. It is now read-only.
Refactor read and write locks to minimize lock duration #251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Addresses issue #249 There are some lock acquisition behavior that exists in `service.rs` which leads to non-obvious lock retention lifetimes. The syntactic sugar pattern matching that rust allows ends up hiding the lifetime acquisition of references to a locked global state. These locks end up getting held across async boundaries unnecessarily which will only lead to lock contention when it is not needed. Additionally, in at least one case, the lock acquisition lifetimes will result in a deadlock as a `write_arc` is being attempted when the write lock is already being held in a parent closure: `service.rs:630`. In order to improve the lock contention and to avoid the deadlock potential the lock guards have been made explicit and placed behind a closure ensuring that it will be cleaned up as soon as the information needed from it is accessed. Any information outside of the lock is cloned in order to ensure no further lock is required. The data that has been cloned / copied out of the lock fields was already being cloned / copied regardless. The end result should be an improved representation of the lock guard's lifetimes, and clarity of the closures that hold a lock guard.
shenkeyao
reviewed
Aug 23, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #249
This PR:
Addresses locks being held across
asyncboundaries by instead wrapping the locks within a short-lived closure. Additionally, it acquires the locks in a separate statement to make it clear that the lock is held for the entire duration of the closure. This is also done for accesses that may not result in a lock outliving the access statement, but is done for clarity and consistency.This PR does not:
Address any locking behavior outside of
service.rs