-
Notifications
You must be signed in to change notification settings - Fork 333
Add true retriever lock #314
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
Conversation
Interceptor_Should_Recognize_Subclass_Of_EasyCachingAble_Attribute test fail. |
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, Thanks!
(long)await _database.EvalAsync(@"if redis.call('GET', KEYS[1]) == ARGV[1] then | ||
return redis.call('DEL', KEYS[1]); | ||
end | ||
return -1;", $"{_name}/{key}", value) >= 0; |
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.
What about move the lua script to a constant?
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.
This unit test is a bit unstable. I'll fix it later. |
Please change the target branch from |
public async Task Concurrency_Test() | ||
{ | ||
ThreadPool.SetMinThreads(1000, 1000); | ||
|
||
var random = new Random(); | ||
var key = Guid.NewGuid().ToString("N"); | ||
var counter = 0; | ||
|
||
using var @lock = Factory.CreateLock("test", Guid.NewGuid().ToString()); | ||
|
||
await Task.WhenAll(Enumerable.Range(0, 100) | ||
.Select(_ => Task.Run(async () => | ||
{ | ||
for (var index = 0; index < 1000; index++) | ||
{ | ||
var value = Interlocked.Increment(ref counter); | ||
|
||
try | ||
{ | ||
Assert.True(await @lock.LockAsync(15000, CancellationToken.None), value.ToString()); | ||
|
||
await Task.Delay(random.Next(0, 10)); | ||
|
||
await @lock.ReleaseAsync().ConfigureAwait(false); | ||
} | ||
catch (DistributedLockException) | ||
{ | ||
} | ||
} | ||
}))); | ||
} |
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.
What is the purpose of this test?
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.
Except repeat lock, all case should be success.
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
{ | ||
return BaseGet(cacheKey, dataRetriever, expiration); | ||
{ | ||
if (_lockFactory == null) return BaseGet<T>(cacheKey, dataRetriever, expiration); |
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.
There are also calls to the BaseGet()
at the EasyCaching.LiteDB.DefaultLiteDBCachingProvider.BaseGetAsync()
var item = dataRetriever(); | ||
if (item != null || _options.CacheNulls) | ||
{ | ||
BaseSet(cacheKey, item, expiration); |
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.
Would it be more appropriate to call Set()
here to record the diagnostic log?
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.
BaseGet have no diagnostic log
/// </summary> | ||
/// <param name="services">Services.</param> | ||
public void AddServices(IServiceCollection services) => | ||
services.Replace(ServiceDescriptor.Singleton<IDistributedLockFactory, CSRedisLockFactory>(x => |
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.
Would it be more appropriate to use AddSingleton
here, and configure DistributedLockName
for each cache using option? (similar to SerializerName
)
services.AddEasyCaching(x =>
x.UseCSRedis(options =>
{
options.DBConfig = new CSRedisDBOptions
{
ConnectionStrings = new System.Collections.Generic.List<string>
{
"127.0.0.1:6388,defaultDatabase=13,poolsize=10"
}
};
options.DistributedLockName = "dlf";
}).WithCSRedisLock("dlf"));
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.
I fix this issure
Local lock: MemoryLock
Distributed Lock: CSRedisLock, MemcachedLock, RedisLock