Every request waiting for the lock and blocking other requests about aspnetcoreratelimit HOT 14 CLOSED

@stefanprodan question is why do you have to set/create a new counter every time? You just want to increment the counter right?
Memory cache is just keeping data in memory. You can just use Interlocked.Increment on the entry.Value.TotalRequests field, and it would work totally thread safe.

from aspnetcoreratelimit.

I believe that building a named locker that locks on counter id is a better solution. the following blog post explains how to implement it http://johnculviner.com/achieving-named-lock-locker-functionality-in-c-4-0/

from aspnetcoreratelimit.

That lock is applied for the get/set of the rate limit key, it doesn't affect the rest of the request processing. How is that related to sync/async?

from aspnetcoreratelimit.

Req1 goes through middleware say IpRateLimitMiddleware => which is calling processor.ProcessRequest(identity, rule);
=> which calls at _core.ProcessRequest(requestIdentity, rule); => which is acquiring a lock on a static object _processLocker at lock (_processLocker)

Now Req2 comes along flows through the same path as Req1 and waits for Req1 to release the lock at lock (_processLocker)

Isn't that making all requests to be bottle-necked at that lock part?

Imagine If we are making a network call to fetch the _counterStore.Get(counterId); from some sort of distributed cache, the impact will much more as the lock held time of each request will be much higher (say 10ms), and if the machine receives some 100 requests at the same instant, then the 1st request will be done in a minimum of 10ms , second will be a minimum of 20ms , 3rd 30ms.. 100th request 100*10ms (and this will keep growing) as each request will be competing to get a lock on that static object?

Correct me if I'm wrong.

from aspnetcoreratelimit.

That lock is there for the local cache, that's in memory in the same process. If you switch to Redis as distributed cache, then there is no need for that lock because Redis has atomic get/set operations. When the Redis provider for .NET Core gets stable I will add a distributed cache implementation to AspNetCoreRateLimit lock free.

from aspnetcoreratelimit.

Got it .. But still even for the in-memory local cache it will slowdown the requests and if a DOS attack happens then all the requests response time will be affected heavily. As throttling is used to prevent such things but in this case with the lock being on a static object all the requests are getting bottle-necked even the genuine ones.

I feel the lock should be replaced with a concurrent(thread safe) dictionary/map ( or some thing of that sort) implementation where requests don't have to depend on other requests (until unless they are coming from same ip or from same client). and thus safe guarding requests of a non throttling(genuine) client from throttling client(DOS attacker)

from aspnetcoreratelimit.

If you use a thread safe dictionary you will still have a global lock when you lookup in that dictionary, otherwise how is it thread-safe? Also that dictionary will grow in time and the lookup will have a greater impact on your app than a simple lock. A thread-safe dictionary has no expire mechanism so you will end up storing there all the IPs and users ids that have no use to the rate limiter.

from aspnetcoreratelimit.

I see that the concurrent dictionary doesn't lock on the whole array, just a part of it. So it could improve the lock time a little, but I don't see how you can clean the dictionary since it has no expire feature.

from aspnetcoreratelimit.

HttpRuntime.Cache might be the answer here instead of plain concurrent dictionary, would take of expiry as well.. see http://stackoverflow.com/questions/33969/best-way-to-implement-request-throttling-in-asp-net-mvc

from aspnetcoreratelimit.

I'm using ASP.NET Core cache that's similar to HttpRuntime.Cache. I think you should look at what operations I am doing inside the lock. ASP.NET cache and HttpRuntime cache have no atomic increment feature.

from aspnetcoreratelimit.

@stefanprodan Core's MemoryCache is thread safe and uses concurrent dictionary under the hood.
https://github.com/aspnet/Caching

from aspnetcoreratelimit.

Let me explain a little the code inside the lock:

            lock (_processLocker)
            {
                //thread safe operation
                var entry = _counterStore.Get(counterId);
                if (entry.HasValue)
                {
                    // entry has not expired
                    if (entry.Value.Timestamp + rule.PeriodTimespan.Value >= DateTime.UtcNow)
                    {
                        // increment request count (THIS IS THE CRITICAL OPERATION)
                        var totalRequests = entry.Value.TotalRequests + 1;

                        // deep copy
                        counter = new RateLimitCounter
                        {
                            Timestamp = entry.Value.Timestamp,
                            TotalRequests = totalRequests
                        };
                    }
                }

                //thread safe operation
                _counterStore.Set(counterId, counter, rule.PeriodTimespan.Value);
            }

A lock free atomic increment is offered by Redis INCR and as far as know there is no such thing in .NET Core cache.

from aspnetcoreratelimit.

Wouldn't you just use Interlocked.CompareExchange?

https://github.com/Icehunter/litterbox/blob/master/LitterBox.Redis/Models/ConnectionPool.cs#L58

That's the "atomic" in c#, unless you declared it as volatile.

from aspnetcoreratelimit.

Check this out #63

from aspnetcoreratelimit.