Java: StringBuilder, static method and possible synchronization issues

Question

Is it possible for code like this to be not thread safe? It is a static method and we are using an local instance of stringbuilder. I guess the input strings might be held by other objects?

public static String cat(final String ... strings) {

    ...
    ...
    final StringBuilder sb = new StringBuilder(totLength);
    for (int i = 0; i < size; i++) {        
        if (strings[i] != null) {
            sb.append(strings[i]);
        }
    }
    return sb.toString();
}

Answer 1

It's not fully thread safe - because another thread might be changing the same array that was passed in the argument for the strings parameter. This isn't entirely obvious because of your use of varargs, but effectively (in terms of thread safety) the method signature is just:

public static String cat(String[] strings)

It won't cause any exceptions, but you may not see the latest values within the array.

As another alternative, you might see something unexpected if it does see the change. For example, suppose we pass in just a single-valued array where the value is initially "x":

public static String cat(final String ... strings) {    
    ...
    ...
    final StringBuilder sb = new StringBuilder(totLength);
    for (int i = 0; i < size; i++) {
        // strings[0] is currently "x"
        if (strings[i] != null) {
            // But now another thread might change it to null!
            // At that point we'd get "null" as output
            sb.append(strings[i]);
        }
    }
    return sb.toString();
}

In other words, although you'd probably expect to either see "x" or "" as the result, you could see "null". To fix that, you can read each value from the array just once:

final StringBuilder sb = new StringBuilder(totLength);
for (int i = 0; i < size; i++) {
    String value = strings[i];
    if (value != null) {
        sb.append(value);
    }
}

You may still see an array which is half way through being changed (e.g. if one thread is changing {"x", "y"} to {"a", "b"} you may see "xb" as the result), but you won't get a spurious "null".

Answer 2

That should be Thread Safe, as the Strings being passed in are immutable. and assuming you create totLength within the method, everything else is local .

EDIT:

as Jon Skeet points out, there is a chance that the vargs value could be passed in not only as a sequence of Strings (as my answer assumes), but also as a String[]. In the latter case, there is the possibility for the array to be modified by another thread while you are processing.