BeanWrapperImpl's thread-safety [SPR-65]

[spring-projects-issues]

Alef Arendsen opened SPR-65 and commented

Juergen, attached you'll find a testcase (and the fix for it) identifying the problem with the thread-safety of the BeanWrapperImpl and its property editors. I wanted to hand it over to you, since I don't know if the fixed might have implications.

Alef

p.s. posted on the list by Guillaume Poirier

---

Affects: 1.0 RC2

Attachments:

• patch.txt (6.27 kB)
• TestCase.tar.gz (1.60 kB)

[spring-projects-issues]

Alef Arendsen commented

The patch, adding a TestCase in org.springframework.beans and modifying the BeanWrapperImpl to instantiate propertyeditors when needed instead of putting them statically instantiating them only once. Patch delivered by Guillaume Poirier

[spring-projects-issues]

Alef Arendsen commented

Another test case by Guillaume (see comments for his accompanying email)

[spring-projects-issues]

Alef Arendsen commented

Guillaume's mail accompanying the tar.gz

Also, I ran a similar Test Case against XmlBeanFactory with prototypes and custom PropertyEditors, and it failled the test. I assume BeanFactory is supposed to be thread safe since it can be shared in a client-server environnement. Making the method AbstractAutowireCapableBeanFactory.createBean(String, RootBeanDefinition) synchronized fixed my particular test case, but I'm not sure if it's the best way to get around this problem. The test case is attached to this mail, all of the files should go in the package org.springframework.beans.factory.

[spring-projects-issues]

Juergen Hoeller commented

That's an important bugfix! It just affects applications that actually rely on those special default editors, though. Juergen

[spring-projects-issues]

Guillaume Poirier commented

Thank you for fixing the first bug so quickly. However, the second attached file was refering another bug, althought related to the fact that PropertyEditors are not thread safe. It affects BeanFactories with registered Custom Editors. The code that creates non-thread safety pontential is in AbstractBeanFactory, since there's a shared Map of PropertyEditors. The only class that seems to use it in a way that is non-thread safe is AbstractAutowireCapableBeanFactory, in the createBean and autowireConstructor (which is only called by createBean).

The non-thread safe case is really narrow though, even less likely to happen than the issue I reported for BeanWrapperImpl. It is if two differents threads have the factory create two beans from different definitions concurently, using the same PropertyEditor passed to the factory throught factory.registerCustomEditor() (directly or with a post-processor). The test case I provided reproduce the situation using prototypes. For Singleton, I guess the only way for that to happen would be with lazy initialization.

My particular failling test case can be made succesful by synchronizing the method AbstractAutowireCapableBeanFactory.createBean(), I think it would get rid of any situations where the factory can be non-thread safe. However I'm wondering why did sun make the PropertyEditors stateful, thus non-thread safe and instances not easily reusable. It seems to serve no purpose, it could just have been conversion methods returning the converted value directly, it would have made possible for them to cache instances rather than class, and for users to registers instances rather than class.

P.S. I also wanted to thank you for the amazing framework you've done and great support you're providing. :)

[spring-projects-issues]

Juergen Hoeller commented

BeanFactory thread-safety

[spring-projects-issues]

Juergen Hoeller commented

Thanks for the second hint, Guillaume - I didn't notice it at first. I've added minimal synchronization to the BeanFactory implementations, i.e. just when potentially converting property values, and just if there are custom editors registered.

I completely agree regarding the java.beans.PropertyEditor design: The Sun architects could have offered straightforward textToValue and valueToText conversion methods, but unfortunately they didn't.

Juergen