CODEWITHSUNDEEP
javastring
Berlin Brown
Berlin Brown

Reputation: 11734

Java: StringBuilder, static method and possible synchronization issues

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();
}

Upvotes: 4

Views: 4417

Answers (5)

Powerlord
Powerlord

Reputation: 88796

See John Skeet's answer for the array part.

As for the StringBuilder, it should be safe. Because the StringBuilder is created inside the method, each call has its own StringBuilder.

StringBuilder itself is not synchronized, so if it existed before the static method call, you'd be better off using a StringBuffer instead.

Upvotes: 0

akf
akf

Reputation: 39485

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.

Upvotes: 5

Jon Skeet
Jon Skeet

Reputation: 1500785

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".

Upvotes: 11

Steve B.
Steve B.

Reputation: 57314

This would also be threadsafe even if you weren't passing in immutable objects, because

  • you're not altering any of the input parameters
  • the only thing you're changing is limited to the local scope of the method.

Upvotes: 3

CPerkins
CPerkins

Reputation: 9018

No, it's thread-safe as written(*). Strings are immutable, so it doesn't matter if multiple threads call this.

(* for the code you show)

Upvotes: 4

Related Questions