GenericScope with ThreadLocalScopeCache heavy concurrency contention on ReadWriteLock
Symptom:
- Application is not processing as many concurrent requests as configured in message listener container
- Thread dumps of application show many threads WAITING on the same ReadWriteLock.
Situation:
GenericScope is used with ThreadLocalScopeCache to scope some beans to a single message-processing operation. (It is a like a web "RequestScope" but for message listeners) At the end of each message processing operation, GenericScope.destroy() is invoked to end the life cycle of these message-scoped beans and clear the ThreadLocalScopeCache for that thread.
Since GenericScope uses pluggable ScopeCaches, it never occurred to me that it would employ a concurrent locking mechanism that bypassed the scope cache and locks on a global scope. The documentation doesn't mention it. But unfortunately, this is what happens. Whenever an operation is invoked on a bean coming from the scope, GenericScope$LockedScopedProxyFactoryBean acquired a read lock on the name of the bean, instead of on the instance of the bean.
ReadWriteLock readWriteLock = this.scope.getLock(this.targetBeanName);
Even though the beans from a ThreadLocalScopeCache can only ever be seen by the thread to which they are scoped, all calls on all threads to the same bean share the same lock. This wouldn't cause any contention in itself, but the reason for acquiring the read lock, is that calls to the bean should be disallowed while the target of the bean is being destroyed. That's why the destroy() and destroy(String beanName) methods in GenericScope lock the corresponding write lock for the bean name.
Lock lock = this.locks.get(wrapper.getName()).writeLock();
Unfortunately that effect of that is that even if only one Threads' ThreadLocalScope is being destroyed at the end of a message, it still write-locks all beans in all threads at the same time, heavily reducing concurrency. In fact, for a ThreadLocalScope, no locking is required at all, as no concurrent access to the beans in the scope is possible.
This leads me to the conclusion that this locking behaviour should be implemented in the implementation of ScopeCache, not in GenericScope, as the required behaviour varies with the ScopeCache implementations.
The locking-behaviour was introduced in 2017 in this commit: https://github.com/spring-cloud/spring-cloud-commons/commit/c28ca5de91408e6c64a2c9a3e1977b8f104940ac#diff-afa0715eafc2b0154475fe672dab70e4
The description suggests that it was done to fix a problem in a subclass of GenericScope: RefreshScope.
Unfortunately this had the effect of making the GenericScope much less generic, and the combination of GenericScope with a ThreadLocalScopeCache not very usable anymore.
Workarounds:
In our case, the beans currently in the scope are simple and do not require life cycle callbacks (yet), so we can get by with just invoking ThreadLocalScopeCache.clear() and not doing life cycle management on the beans.
Another workaround would be to copy the code of GenericScope and removing the locking behaviour.
Solutions:
The details of the problem in RefreshScope that were fixed with the introduction of locking are not clear to me, but it seems to me that the locking behaviour should be in the ScopeCache implementation, in this case StandardScopeCache, so that the GenericScope can still be meaningfully used with other ScopeCache implementations that do not require locking.
@dsyer thoughts?
The analysis looks good. Do we have a pull request?