Is private method in spring service implement class thread safe

Question

I got a service in an project using Spring framework.

public class MyServiceImpl implements IMyService {
    public MyObject foo(SomeObject obj) {
       MyObject myobj = this.mapToMyObject(obj);
       myobj.setLastUpdatedDate(new Date());
       return myobj;
    }

    private MyObject mapToMyObject(SomeObject obj){
       MyObject myojb = new MyObject();
       ConvertUtils.register(new MyNullConvertor(), String.class);
       ConvertUtils.register(new StringConvertorForDateType(), Date.class);
       BeanUtils.copyProperties(myojb , obj);
       ConvertUtils.deregister(Date.class);
       return myojb;
    }
}

Then I got a class to call foo() in multi-thread;

There goes the problem. In some of the threads, I got error when calling

BeanUtils.copyProperties(myojb , obj);

saying Cannot invoke com.my.MyObject.setStartDate - java.lang.ClassCastException@2da93171

obviously, this is caused by ConvertUtils.deregister(Date.class) which is supposed to be called after BeanUtils.copyProperties(myojb , obj);.

It looks like one of the threads deregistered the Date class out while another thread was just about to call BeanUtils.copyProperties(myojb , obj);.

So My question is how do I make the private method mapToMyObject() thread safe?

Or simply make the BeanUtils thread safe when it's used in a private method.

And will the problem still be there if I keep the code this way but instead I call this foo() method in sevlet? If many sevlets call at the same time, would this be a multi-thread case as well?

Show the full class package/class names of ConvertUtils and BeanUtils. Are they from the spring framework? Are they from the apache commons libraries? — Erwin Bolwidt, Aug 22 '14 at 04:48
possible duplicate of [Must Spring MVC Classes be Thread-Safe](http://stackoverflow.com/questions/16795303/must-spring-mvc-classes-be-thread-safe) — Raedwald, Aug 22 '14 at 06:39
Servlet containers may handle different requests in different threads, therefore if your servlet (Spring) code shares state between different requests *you* must ensure this is done in a thread safe manner. See http://stackoverflow.com/questions/16795303/must-spring-mvc-classes-be-thread-safe — Raedwald, Aug 22 '14 at 06:42
@Erwin Bolwidt sorry man, It's from org.apache.commons.beanutils — Roger Ray, Aug 22 '14 at 07:24

Gustav Grusell · Accepted Answer · 2014-08-22T08:18:29.643

4

Edit: Removed synchronized keyword since it is not neccessary, see comments below.

Instead of using the static methods in the BeanUtils class, use a private BeanUtilsBean instance (http://commons.apache.org/proper/commons-beanutils/apidocs/org/apache/commons/beanutils/BeanUtilsBean.html). This way, you don't need to register/deregister your converters each time the method is called.

public class MyServiceImpl implements IMyService {
    private final BeanUtilsBean beanUtilsBean = createBeanUtilsBean();

    private static BeanUtilsBean createBeanUtilsBean() {
        ConvertUtilsBean convertUtilsBean = new ConvertUtils();
        convertUtilsBean.register(new MyNullConvertor(), String.class);
        convertUtilsBean.register(new StringConvertorForDateType(), Date.class);
        BeanUtilsBean beanUtilsBean = new BeanUtilsBean(convertUtilsBean);
        return beanUtilsBean;
    }

    public MyObject foo(SomeObject obj) {
       MyObject myobj = this.mapToMyObject(obj);
       myobj.setLastUpdatedDate(new Date());
       return myobj;
    }

    private MyObject mapToMyObject(SomeObject obj){        
        MyObject myojb = new MyObject();      
        beanUtilsBean.copyProperties(myojb , obj);
        return myojb;
    }
}

edited Aug 22 '14 at 08:18

answered Aug 22 '14 at 06:01

Gustav Grusell

1,166
6
7

Thanks for you answer. But are you sure beanUtilsBean is thread safe by make it final? Do I still need to deregister the Date class each time after call the `beanUtilsBean.copyProperties(myojb , obj)` method? – Roger Ray Aug 22 '14 at 06:18
The final keyword does not make BeanUtilsBean thread safe. The synchronized block on the mapToMyObject guarantees that the thread safety of the BeanUtilsBean is not an issue, since it will only be used by one thread at the time. There is no need to deregister the Date class since the ConvertUtilsBean is a private instance. – Gustav Grusell Aug 22 '14 at 06:32
`apache-commons-beanutils` is intended to be threadsafe. It's unfortunately not clearly stating it on their website. But in the [release notes](http://commons.apache.org/proper/commons-beanutils/javadocs/v1.9.0/RELEASE-NOTES.txt) you can see several bug fixes for thread-safety issues which indicates that they want it to be thread-safe. So in that case, the `synchronized` modifier on the method is not necessary and the code can execute more optimally in a multi-threaded environment. – Erwin Bolwidt Aug 22 '14 at 07:41
@ErwinBolwidt, thanks for the info, that is good to know. I had a quick look through the docs, but like you say they do not mention threadsafety so added the synchronized keyword to ensure thread safety. – Gustav Grusell Aug 22 '14 at 08:17

score 0 · Answer 2 · answered Aug 22 '14 at 04:17

0

add a synchonized block to the sensitive portion of your code or synchronize the method:

http://docs.oracle.com/javase/tutorial/essential/concurrency/sync.html

answered Aug 22 '14 at 04:17

75inchpianist

4,112
1
21
39

That only works if there are no other places in the code that register or deregister converts. Changing global converter configuration temporarily is not a good idea at all; the OP should look for a different mechanism to copy the properties. – Erwin Bolwidt Aug 22 '14 at 04:55

Is private method in spring service implement class thread safe

2 Answers2