Necessity of synchronized on local variable

Question

In the JSON-java library (org.json.JSONArray) I have found this code snippet with a synchronized block around a method-local variable

public String toString(int indentFactor) throws JSONException {
    StringWriter sw = new StringWriter();
    synchronized (sw.getBuffer()) {
        return this.write(sw, indentFactor, 0).toString();
    }
}

I do not understand the necessity for the synchronization here, as the StringWriter is only local to the given method (and, why the synchronization is on the Buffer). Is the synchronization really necessary here, and if, why?

Vip

It is unnecessary. With the default constructor, the buffer is a newly instantiated object. buf = new StringBuffer(); inside the constructor. – Raising

Note that the author of the org.json library is a JavaScript programmer, not a Java programmer, and this fact is demonstrated throughout the code. For serious JSON processing, consider using Google's Gson library. – Ingrowing

Answer 1

This may be a performance optimization. In the oracle jvm, re-acquiring an already held lock is very fast. Presumably, the write call is making numerous calls into the StringBuffer. By locking before calling write, the lock will be held through all of those calls instead of being released and re-acquired for each call.

Rahmann

As of Java 6, lock biasing means that acquiring an uncontended lock is fast anyway. – Ingrowing

@ChrisJester-Young - yes, i don't know how relevant that particular performance optimization still is (or ever was). – Rahmann

I don't understated being released and re-acquired for each call. The write() call doesn't try and synchronize on the Writer at all. The lock gets acquired and released only once, inside the toString(int) method in OP's question. – Raising

@SotiriosDelimanolis - all the methods of StringBuffer (which underlies the StringWriter) are synchronised. – Rahmann

Answer 2

The empty constructor for StringWriter is

/**
 * Create a new string writer using the default initial string-buffer
 * size.
 */
public StringWriter() {
    buf = new StringBuffer();
    lock = buf;
}

Nothing is shared, therefore the synchronized block is unnecessary.

Unless... write delegates to another thread, but I seriously doubt that.

Raising

Answer 3

getBuffer() returns a StringBuffer, and according to the documentation a StringBuffer is already synchronized:

String buffers are safe for use by multiple threads. The methods are synchronized where necessary so that all the operations on any particular instance behave as if they occur in some serial order that is consistent with the order of the method calls made by each of the individual threads involved.

This means that synchronizing again on the StringBuffer is totally overkill. Changes to the StringWriter will automatically be synchronized because it uses the synchronized StringBuffer internally.

Since the StringWriter instance is local to the method call, it's impossible to have multiple threads accessing the same instance at the same time, which also makes synchronization unnecessary.

Ava

Answer 4

It is a bug. Every thread creates its own local variable in the method and synchronizes on it.Each time entering the method threads create their own object-monitor which cannot be held by another thread because it's local and lives only on thread's stack!

Seigneury