BEANUTILS-541 - FluentPropertyBeanIntrospector caches corrupted writeMethod (parallel) by seregamorph · Pull Request #234 · apache/commons-beanutils

Once #68 was merged I realized it can work not properly in a concurrent environment 🤦.
I made some tests and it was pretty hard to trigger failure - the merged fix is relatively resistant and a combination of random factors should happen simultaneously, also it only happened with high number of threads. But finally I got proof there is a problem still and tried to find a better solution without erasing the cache.

So, how the updated implementation works: now there is no more PropertyDescriptor.setWriteMethod and the shared cached PropertyDescriptor object is not updated anymore as there is a risk to define a setter method for a field which is wrong in case if getter is defined in super-class and it has several subtypes defining their own setters (with generic types).
Instead, in the IntrospectionContext the PropertyDescriptor is just replaced with a copy of the shared object with configured setter. This should be valid, because IntrospectionContext is not a cached type. If you run Jira541TestCase.testFluentBeanIntrospectorOnOverriddenSetter in debugger, you can see that at line FluentPropertyBeanIntrospector:163 which makes this call

PropertyDescriptor fluentPropertyDescriptor = new PropertyDescriptor(
        pd.getName(), pd.getReadMethod(), m);
// replace existing (possibly interited from super-class) to one specific to current class
icontext.addPropertyDescriptor(fluentPropertyDescriptor);

the existing object is replaced with the copied by the same name.

Sorry for missing this initially, I just got this insight. But I'd like to notice that initial fix (merged yesterday) makes the situation much better than it was.
I hope you will find time to check this carefully in the debugger and see the specific how the fluent setters logic is working.

Thanks for your attention and trust.

Hint: to reproduce the failure, run testFluentBeanIntrospectorOnOverriddenSetterConcurrent (not the full test class, but a single method) with an old implementation (I mean current master) of FluentPropertyBeanIntrospector several times. Eventually it will fail with:

java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: org.apache.commons.beanutils2.bugs.Jira541TestCase$SubTypeB is not assignable from org.apache.commons.beanutils2.bugs.Jira541TestCase$SubTypeA

	at java.base/java.util.concurrent.FutureTask.report(FutureTask.java:122)
	at java.base/java.util.concurrent.FutureTask.get(FutureTask.java:191)
	at org.apache.commons.beanutils2.bugs.Jira541TestCase.testFluentBeanIntrospectorOnOverriddenSetterConcurrent(Jira541TestCase.java:58)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)

One more reason why we need this change: library should not call PropertyDescriptor.setWriteMethod(m); for a descriptor instance which is stored in a static cache, because this cached instance can potentially be reused by other frameworks (like Tomcat or JEE frameworks) when analyzing bean contracts. After calling setWriteMethod the behavior in other frameworks may easily change and rely on random factor - lead to flaky issues. Overall, I believe it was a JDK design mistake to make PropertyDescriptor mutable and statically cacheable at the same time 😢