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.

Also you ensure that you do not forget the `synchronized` block if you latter use the method from another code in your class. I would clearly put it in the method. — SJuan76, Jan 02 '13 at 11:18
Side note: synchronized and "heavy stuff" often don't go well together... — assylias, Jan 02 '13 at 11:21
@Mat: You'r right: Its a classical check-than-act problem. I changed the example. — scheffield, Jan 02 '13 at 12:10
@assylias: indeed, the productive code dose something sequentially and not heavy. But can you explain - in short - why synchronized and "heavy stuff" often don't go well together? — scheffield, Jan 02 '13 at 12:10
@scheffield the synchronized block is executed sequentially - if what you do inside takes time, this can quickly become a bottleneck where threads will spend a lot of time waiting for the monitor. In general, you want to keep synchronized section as small as possible. — assylias, Jan 02 '13 at 12:53

score 3 · Accepted Answer · answered Jan 02 '13 at 11:23

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.

score 1 · Answer 2 · answered Jan 02 '13 at 11:17

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.

score 0 · Answer 3 · answered Jan 02 '13 at 11:17

0

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

answered Jan 02 '13 at 11:17

mindas

26,463
15
97
154

2

No, the link is specifically about double-checked locking. – Marko Topolnik Jan 02 '13 at 11:19
Correct, but the abstract class in the article has got method `invalid(X value)` which is similar case to OP and easy to retrofit. I have clarified my answer though. – mindas Jan 02 '13 at 11:22
Main point: if *precondition* doesn't depend on the state that is protected by the lock (and mutated by `doHeavyStuff`), then the double-checked idiom is superfluous. – Marko Topolnik Jan 02 '13 at 11:24

Should synchronized accessed methods be synchronized?

3 Answers3