Should synchronized accessed methods be synchronized?

Question

I want to access a method from within an synchronized block. Here is an example:

public void doSomething() {
    // simple stuff

    // a block to reduce the synchronized code to
    // what really needs to be synchronized.
    synchronized(this) {
        if (precondition) {
            doSequentialStuff();
        }
    }
}

private void doSequentialStuff() {
    // do stuff needs to be performed sequentially.
}

To write clean code I wondered whether it would be good to make the method doSequentialStuff explicitly synchronized. IMHO this would make no difference in semantic since the lock is in both cases this and the method is guaranteed to be accessed only from the synchronized block. I hope to increase the readability.

Any advice?

Edit: I modified the example to incorporate the comments.

Answer 1

If there is no legitimate code path by which doHeavyStuff may be executed without holding the lock, then by all means make it synchronized in order to preempt any future bugs introduced by an unwary developer. The readability of code can only improve what way.

Answer 2

It's probably better to go with an assert to check that the lock is being held. Note, you do need assertions enabled for the check to be performed.

assert Thread.holdsLock(this);

Generally if you are using this sort of private method it tends to indicate that you should split the class into two. An outer layer does the locking and perhaps other things appropriate for the client, whereas a deeper layer is more concerned with implementation.

Use of this to lock is dubious. Generally it's better to use a private explicit lock object.

Answer 3

Have a look at http://weblogs.java.net/blog/mason/archive/2006/09/rechecking_doub.html, it has got similar pattern covered (uses singleton as an example, but you can easily retrofit it for your case).