Invoking sequence of calls in the lock, two cases

Question

I have a class with synchronized method in it:

class A {
    synchronized void method1() {};
    synchronized void method2() {};
}

IF some client class wants to invoke method1() then method2() knowing that those calls are atomic

First case:

class B {    
    A a;    
    public void foo() {    
        synchronized(a) {
            a.method1();
            a.method2();
        }    
    }

}

Second case:

 class B {    
        A a;

        final Object lock = new Object();

        public void foo() {    
            synchronized(lock) {
                a.method1();
                a.method2();
            }    
        }

    }

Is there any reason why I should use one case but not another?

Must the combination of method1() and method2() be atomic, or is it sufficient that each method separately is atomic? Are there cases where only method1() or method2() would be called? — Tom McIntyre, May 24 '13 at 19:44
Supposed that A class should be thread safe for any other clients which could invoke only one method separately - method1() or method2(). — ses, May 24 '13 at 19:47

score 1 · Answer 1 · answered May 24 '13 at 19:56

1

It probably makes most sense to synchronize on a. That at least maintains the principle of 'least surprise'. a is the most natural object for other clients to synchronize on as well. Add some documentation to make that clearer. Plus you are guaranteed to get the lock on the synchronized methods in A, which may be slightly more efficient than potentially contended locking if you are using different locks in A and B.

answered May 24 '13 at 19:56

Tom McIntyre

3,620
2
23
32

Synchronizing on a is definitely the right way to enforce atomic calls to method1 and method2. Using a second lock introduces the possibility of deadlocks. Also, the code as shown using the second lock won't work, because the lock is only used in one place. Java locking requires that all code paths using protected code acquire the same lock (or set of locks). – bithead61 May 24 '13 at 20:04
1

@bithead61 How would you deadlock, _if_ (as in the code above) the hold on `lock` is always acquired before the hold on `a`? Deadlocks require that locks be acquired in different order, not just that there are multiple locks. – yshavit May 24 '13 at 20:07
@yshavit Yes, deadlocking would require the locks to be acquired in a different order, which is why I said adding the second lock introduces the _possibility_ of deadlocks. However, as I said, for the second lock to have any effect, it would have to be used in more than one place, thus requiring scrutiny of each call to ensure that locks are acquired in the right order. – bithead61 May 24 '13 at 20:27
@bithead61 Okay, fair enough. But then, the possibility of deadlock is there even without a second lock. `synchronized void method1() { method2(); } synchronized void method2() { method1(); }` -- potential deadlock! – yshavit May 24 '13 at 20:36
@yshavit No, locking one lock twice does not introduce the possibility of deadlocks, because the locks used on synchronized blocks are _reentrant_ locks. See http://stackoverflow.com/questions/249867/synchronising-twice-on-the-same-object – bithead61 May 24 '13 at 21:03
@bithead61 Let me fix that, `synchronized void method1() { friend.method2(); }` where `friend` is also an `A`. http://pastebin.com/DEjPDzfr as an sscce. Another way to put it is to point out that the original example _already_ had multiple locks, even without the extra `lock` object. Also, in the original post, the second synch on `lock` will have an effect, provided all invocations of `method1/2()` for a given `a` go through the same `b`. – yshavit May 24 '13 at 21:24

yshavit · Answer 2 · 2013-05-24T20:03:17.550

If you always invoke the methods using the first technique, then the two methods are really atomic: only one thread at a time will (run method1 and run method2), as a block.

In the second case, any given instance of B will invoke the two methods atomically -- but if two threads each have a separate instance of B, then the method1() and method2() calls can interleave. Imagine:

Thread1 owns B b1 = new B(a)
Thread2 owns B b2 = new B(a) (for the same instance a)
Thread1 invokes b1.newB(), acquires a lock on b1.lock
Thread2 invokes b2.newB(), acquires a lock on b2.lock (which is not contended, since it's a separate object than b1.lock)
Thread1 starts a.method1()
Thread2 tries to start a.method1() but is blocked because it's synchronized
Thread1 finishes a.method1()
Thread2 starts a.method1()
Thread1 starts a.method2() as Thread2 is still running method1()
- Atomicity is not achieved!

You could address this by making B.lock a static final, but this may be more locking than you need -- now all calls to {A.method1(); A.method2();} are serialized, rather than being serialized per instance of A.

Even if you use first synchronization the method, you're still at the mercy of everyone else playing nice. If someone else invokes a.method1() directly (not via B.foo), you can still get non-atomic interleaving.

Thread1 owns B b1 = new B(a)
Thread2 owns a directly (same instance of a)
Thread1 invokes b1.newB(), acquires a lock on b1.lock
Thread1 starts a.method1()
Thread2 tries to start a.method1() but is blocked because it's synchronized
Thread1 finishes a.method1()
Thread2 starts a.method1()
Thread1 starts a.method2() as Thread2 is still running method1()
- Atomicity is not achieved!

There's not much you can do about that, other than restricting the visibility of method1() and method2(). For instance, you could make them package-private or protected, and assume/hope that classes that have access to the methods will know better than to abuse their powers. If you go that route, then you don't even need to synchronize method1() or method2() -- you can document and assume (and even assert, if you want) that they'll be invoked while this is locked.

Invoking sequence of calls in the lock, two cases

2 Answers2