3

Let's say I have following method

public static String addStringItems(String[] items, boolean forceUpperCase) {
    StringBuilder builder = new StringBuilder(items.length);
    for (String item : items) {
        builder.append(item);
    }
    return forceUpperCase ? builder.toString().toUpperCase() : builder.toString();
}

Now, how do I make it thread-safe, is marking it synchronized enough?Or should I use StringBuffer instead of StringBuilder?Do you have any other suggestions?

Petr Mensik
  • 26,874
  • 17
  • 90
  • 115

1 Answers1

8

Your method is well-written and as thread-safe as it can be. It accesses no shared state and is basically a pure function. The method is passed an array, which is itself a non-threadsafe object, however that concern should not be taken into consideration when assessing the method's own thread safety.

The key thing to note is that if the caller didn't take care of the thread-safety issues with the String[], then there is nothing that this method can do about it, even if it tries to make a defensive copy within a synchronized block. There is no guarantee that the lock used by this method would be used to protect the String[] at all other places where it may be modified. This is why the concurrent consistency of the String[] argument is fundamentally the caller's responsibility.

By the same token, there is no need to use a StringBuffer, which only makes sense when shared between threads. Your StringBuilder never leaves the method and the method is executed on the same thread in its entirety.

Marko Topolnik
  • 195,646
  • 29
  • 319
  • 436
  • For reference, see also http://stackoverflow.com/questions/12825847/why-are-local-variables-thread-safe-in-java – Martin Klinke Jan 19 '14 at 11:51
  • Ahh, yes, you are right I just wasn't sure if I am 100% correct on my assumptions. Thanks a lot – Petr Mensik Jan 19 '14 at 11:53
  • To be more precise, the method could access shared state as it receives a reference to a string array (which might also be accessed by other threads, depending on what the application does). It is thread safe because it doesn't modify it. – Lefteris Laskaridis Jan 19 '14 at 11:59
  • @lefty If another thread concurrently modified the array, that would cause unpredictable behavior. However, that is a wider concern which should not be handled by this method. – Marko Topolnik Jan 19 '14 at 12:01
  • 1
    @Marko Sorry but i must disagree on that. The method should not assume anything concerning the array's thread safety and create a copy to work with instead of the original array. This would solve any concurrency issues, including visibility which is the main cause of unpredictable behaviour here. – Lefteris Laskaridis Jan 19 '14 at 12:05
  • @Marko Since we're talking about a method that receives a reference to an object that doesn't own (i.e. alien method), and since the issue here is making that method thread safe, then it simply should not make any assumptions on how this instance is used and protect against any concurrency issues. Anyways, I'm sorry you feel angry about that. – Lefteris Laskaridis Jan 19 '14 at 12:14
  • @Marko Well, the intention is not to protect the String[] from modification in the first place. Instead it's to protect addStringItems from potential side-effects if String[] changes by another thread during the method's execution. – Lefteris Laskaridis Jan 19 '14 at 12:31
  • @lefty You *cannot protect* the `String[]` from changes by another thread during the method's execution, as I have detailed in the sentence I have added since your downvote. If you think it is possible, please make a clear proposal on how it can be done and help me write an even better answer. – Marko Topolnik Jan 19 '14 at 12:34