Synchronized consume method in AbstractCacheRateLimiter impact performance

[addozhang]

Currently we are using 1.7.2 which is similar with AbstractCacheRateLimiter in latest version. The repository we use is redis.

We did performance test. With 1.7.2, the TPS is 600+. After remove synchronized , it is up to 2000+.

In AbstractCacheRateLimiter, it has a synchronized method. In redis implementation, there is no object shared among threads. This method is invoked by each incoming request.

[lchayoun]

@addozhang
I think that the synchronized method is important otherwise we might miss consumptions.

[addozhang]

But in RedisRateLimiter, both RateLimiterErrorHandler and RedisTemplate are thread-safe. It is not necessary to use synchronized.

[lchayoun]

Hi, @addozhang we recently changed the code in #108 and it should has better performance.
Can you test it?

[marcosbarbero]

Closing due inactivity

[FangXiaoMing2021]

@lchayoun Why you think remove synchronized method, it might miss consumptions.

[addozhang]

@Fangfeikun Let me answer it. synchronized is usually used in race condition. ZuulFilter should be stateless object among requests. RateLimiter object used in RateLimitPreFilter should be stateless object too. Otherwise, all requests in this filter will wait for the monitor of RateLimiter.

[addozhang]

@lchayoun Sorry for late reply.
I am confused. What is protected by synchronized? In RedisRateLimiter, both RateLimiterErrorHandler and RedisTemplate are thread-safe. There are no objects cannot be shared among threads.

[taxus1]

@addozhang How do you solve this? remove synchronized key word has any problem?

[addozhang]

@taxus1 yes, by removing synchronized and no any other problem found.

[taxus1]

@addozhang thanks