Skip to content
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

BEANUTILS-541 - FluentPropertyBeanIntrospector caches corrupted writeMethod (parallel) #234

Merged
merged 2 commits into from
Apr 14, 2024

Conversation

seregamorph
Copy link
Contributor

@seregamorph seregamorph commented Apr 14, 2024

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 (for this reason I believe 1.x fix can stay as is). 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 😢

@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.13%. Comparing base (99578cc) to head (24f8c74).
Report is 44 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #234      +/-   ##
============================================
+ Coverage     65.06%   65.13%   +0.07%     
- Complexity     1484     1515      +31     
============================================
  Files           105      111       +6     
  Lines          5504     5647     +143     
  Branches       1068     1086      +18     
============================================
+ Hits           3581     3678      +97     
- Misses         1464     1490      +26     
- Partials        459      479      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@garydgregory garydgregory merged commit a726f85 into apache:master Apr 14, 2024
8 checks passed
@garydgregory
Copy link
Member

@seregamorph
Thank you for circling back and the follow up PR.

@seregamorph seregamorph deleted the BEANUTILS-541-concurrent branch April 14, 2024 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants