How to make the static method thread safe in JAVA?

Question

I am creating a web application and meet a thread safe problem. After reading several similar questions, I am still confusing about my case. I am using the java spring framework to setup the REST web service. All the request (JSON of Person Object) will be pass to the checkIfGoodName function, like Checker.checkIfGoodName(person). They are all static method call. I am wondering that, is this function, Checker.checkIfGoodName THREAD SAFE ? If no, how to modify the code ? I have code as below:

Checker.java

public class Checker {

    public static void checkIfGoodName(Person person){

        checkingName(Person person);

    }

    private static void checkingName(Person person){

        if(person.getName()==null){
            PersonUtils.addErrorMessage(Person person, new String("Name is empty"));
        }

    }

}

PersonUtils.java

public class PersonUtils {

     public static void addErrorMessage(Person person, String errorMessage){

         List<Message> msg = person.getMessageList();
         if(msg!=null){
             msg.add(buildMessage(errorMessage));
         }
     }

     public static void buildMessage(String errorMessage){
         if(errorMessage != null){
             Message msg = new Message();
             msg.setMsg(errorMessage);
         } 
     }

}

Solution 1

Don't think about making methods thread safe. Thread safety is about protecting the integrity of data. More specifically, it is about preventing threads from accessing data when some other thread is in the process of changing the data.

Your PersonUtils.addErrorMessage(person, message) method modifies List instances belonging to Person instances. Access to the lists should be synchronized If the same list can be modified by two different threads, or if it can be modified by one thread and accessed by other threads.

Adding an item to a list takes several steps, and the list almost certainly will appear to be in an illegal state if thread A is able to see it at a point when thread B has performed some, but not all of the steps. It's worse if two threads attempt to modify the list at the same time: That could leave the list in a permanent, illegal state.

You would still need synchronization even if threads that operate on the same Person instance don't actually do it at the same time. The reason is, without synchronization, the computer hardware and the operating system do not guarantee that changes made in memory by one thread will immediately be visible to other threads. But, synchronized comes to your rescue:

Whatever changes thread A makes before it leaves a synchronized(foo) block will be visible to thread B after thread B enters a synchronized(foo) block.

The simplest thing for you to do, again if different threads access the same Person instance, would be to synchronize on the Person object in your addErrorMessage(...) method:

  public static void addErrorMessage(Person person, String errorMessage){
      synchronized(person) {
          List<Message> msg = person.getMessageList();
          if(msg!=null){
             msg.add(buildMessage(errorMessage));
          }
      }
  }

Solution 2

I don't see PersonUtils has a state, however it could be the case where you have same Person instance being passed twice simultaneously so better to make a lock on Person instance,

assuming you actually have message list associated with instance and not a static property

also it is not good to associate error message with Model itself, try exploring some standard practices with the framework you mentioned

Comments

[hansvb]

If Person needs locking, then it's probably better to move the critical code into Person itself (seeing that it already has the message list). Or, as you suggest, remove the message list from Person altogether.

[jmj]

remove messageList from Person entirely wins

[hansvb]

"remove messageList": You could have a "PersonValidator" that combines a Person with the message list (and associated logic), keeping what looks like UI state separate from model class. None of these objects likely needs to be shared between multiple threads.

[Terry]

Thanks for your guys' reply. My web service take the person json converting to person class, as the input of the checkIfGoodName, and there are several static methods are called and update the person message member variables. Other request will create different person object due to their json input and passed to the checkIfGoodName. Thus, can I say that, there is not shared object be accessed by multiple thread by the same time, thus there is not thread safety issue for my code ?

[Terry]

Thanks for your reply. Actually the PersonUtils.addMessage(...) add message object into the message list of the person object. so, It mutates the person object. But I can still think it is thread safe because it only mutate the current person object, and other threads mutates its person object. There is not shared person object thus it is thread safe. right ? Thanks

[jmj]

I am still suggesting to separate message from Person

[Solomon Slow]

@Terry, Oops! you are right. I did not read your PersonUtils.addMessage(...) closely enough. I will change my answer.

[Terry]

@JigarJoshi Thanks for your suggestion. The schema of person object is already determined, and my function just need to update the message member object in Person. :-)

[Mikhail]

all variable in the example are local , why we should use synchronization and decrease performance ?

[Solomon Slow]

@Mikhail all of the variables that are explicit in the example are local, but the example calls methods; PersonUtils.addErrorMessage(...), person.getMessageList(), msg.add(...). Can you say for certain that none of those methods touches anything but local variables? What use would they be if that were true?

[Mikhail]

yea. you are right . I looked more closely. in this situation i ll rewite the code and try to avoid using the synchronized