Skip to content
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

changed mutex in cache repository to read write mutex so that all concurrent reads are allowed #182

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

sriharshabm
Copy link
Contributor

@sriharshabm sriharshabm commented Feb 14, 2025

As of now normal mutex is used in cache repository which will make multiple read to wait for mutex. Changed mutex in cache repository to read write mutex so that all concurrent reads are allowed.

Also. Added missing mutex unlock in case of failure

@nephio-prow nephio-prow bot requested review from s3wong and tliron February 14, 2025 08:12
@sriharshabm sriharshabm requested a review from kispaljr February 14, 2025 08:14
@sriharshabm sriharshabm changed the title changed mutex in cache repository to read write mutex so that all con… changed mutex in cache repository to read write mutex so that all concurrent reads are allowed Feb 14, 2025
@mansoor17syed
Copy link
Contributor

This is a good optimization because:

  1. Read operations can now happen concurrently instead of being serialized
  2. Only write operations will block other operations
  3. Fixes a potential mutex leak in error paths

However, let me suggest some review points:

  1. Verify Lock Usage
  2. Ensure all read operations use RLock()/RUnlock()
  3. Ensure all write operations use Lock()/Unlock()
  4. Double check all error paths have proper unlocking

Test Coverage (Could be a separate PR)

  1. Add concurrent read tests to verify multiple reads work simultaneously
  2. Add mixed read/write tests to verify proper locking behavior
  3. Add stress tests to catch any race conditions if possible

@kispaljr
Copy link
Collaborator

/lgtm

Copy link
Member

@liamfallon liamfallon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

It would be good to get this into R4

Copy link
Contributor

nephio-prow bot commented Feb 18, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liamfallon, sriharshabm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nephio-prow nephio-prow bot added the approved label Feb 18, 2025
@nephio-prow nephio-prow bot merged commit 6a6ce21 into nephio-project:main Feb 18, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants