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

Improve the locking per key logic in the CachingService #187

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

theodorzoulias
Copy link

The existing locking implementation in the CachingService class is not robust. It is possible for the policy of exclusive execution per key to be violated, because the lock is released without proper memory fences. The details about when and why this can happen, can be found in this StackOverflow question, where I asked specifically about the locking code of the CachingService class, and was answered by an expert.

Also the existing spinning logic, using the Interlocked.CompareExchange and Thread.Yield APIs, is unlikely to be as efficient as the simple lock statement. So with this pull request I am proposing to replace the existing int[] keyLocks with an object[] keyLocks, initialize the array with a new object() in each slot, and use the normal and trustworthy lock. It should have no observable difference compared with the current behavior, apart from enforcing properly the exclusive execution policy.

Note: The code in this pull request has not been tested, and might have syntax errors.

Improved locking
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant