Why a synchronized StringBuffer was never a good idea

Introduction

StringBuffer is a synchronized class for mutable strings.  The main problem with making it synchronized is that

  1. It was usually used as a local variable so making it synchronized just made it slower.
  2. It was never a good idea to use it in a multi-threaded way.  This problem is that developers assumed that methods which used StringBuffer were themselves thread safe when they were not.

The problem with StringBuffer

This is an example from a real class which is used in production in many trading systems. It's not a commonly used but you might assume that StringBuffer gives you thread safety, when it doesn't.

    private StringBuffer sb = new StringBuffer();

    public void addProperty(String name, String value) {
        if (value != null && value.length() > 0) {
            if (sb.length() > 0) {
                sb.append(',');
            }
            sb.append(name).append('=').append(value);
        }
    }

While individual calls are thread safe, multiple calls are not.  It is almost impossible to find a good use for StringBuffer that doesn't involve multiple calls (including toString)

A puzzle

Imagine three threads call (in no particular order)

T1: addProperty("a", "b");
T2: addProperty("c", "d");
T3: sb.toString();

write a program which will generate every possible output of T3's sb.toString()  

I found 89.  With thread safety, you might reduce this to 4.

Note: if you used StringBuilder it would be worse, but at least you might not assume your method is thread safe when it is not. e.g. SimpleDateFormat uses StringBuffer ;)








Comments

  1. While not disagreeing with the facts you've laid out, I do think perhaps you are misrepresenting things a little; Suppose in your example you substitute StringBuffer with some synchronised Collection, would you also expect multiple calls on the collection to be safe from concurrent access while in a loop? Basically what you want is either a lockable StringBuffer (ReadWriteLock style) or an API similar to AtomicX, e.g StringBuffer#checkLengthAndAppend(int length, Object value).

    So StringBuffer's behaviour is consistent with the other synchronised APIs in core Java is what I'm getting at.

    ReplyDelete
    Replies
    1. It is consistent, but consistent in a way which is more likely to lead to bugs than actually be useful. BTW you can synchronized(stringBuilder) to get a lockable, mutable string buffer. (Not that I think this is terribly sane either, but you can do it)

      Delete
    2. If you understand that your method is not atomic - then the bug will not happen. Also, you should probably be using objects that are immutable which prevents this kind of problem from coming up.

      Delete
    3. You are right as general approach, but in this specific case, you wouldn't use the immutable String as you still need to update something.

      Delete
    4. Quite simple, Sun cocked up on concurrency before Java 5, and never cleaned out all the obvious rubbish for Java 5, so it is still in Java 7, even poor code in 'private' NIO2 classes!

      Most if not all of the attempts at Thread Safety introduced by Sun before the Java Concurrency classes were introduced were naive nonsense e.g. StringBuffer, Vector, Hashmap, Collections.synchronized*(), because most useful atomic operations require multiple operations on an object. Even some of the Java Concurrency classes need to be protected by a lock sometimes, because they lack methods to execute more complex operations atomically.

      Hopefully Oracle will finally clean up this mess in Java 8.

      Delete
  2. Can you explain the '89' to me? How is it be calculated...

    ReplyDelete
  3. The 89 was determined by considering any of the three threads can occur in any order but each operation is safe. Btw if it was not thread safe there would be more possibilities.

    ReplyDelete

Post a Comment

Popular posts from this blog

Java is Very Fast, If You Don’t Create Many Objects

System wide unique nanosecond timestamps

Unusual Java: StackTrace Extends Throwable