trying to grasp basic thread synchronisation in java

Question

public class ThreadTest implements Runnable {

private int counter;
private Date mydate = new Date();

public void upCounter1() {
    synchronized (mydate ) {
        for (int i = 0; i < 5; i++) {
            counter++;
            System.out.println("1 " + counter);
        }
    }

}

public void upCounter2() {
    synchronized (mydate ) {
        for (int i = 0; i < 5; i++) {
            counter++;
            System.out.println("2 " + counter);
        }
    }
}

public void upCounter3() {
    synchronized (mydate ) {
        for (int i = 0; i < 5; i++) {
            counter++;
            System.out.println("3 " + counter);
        }
    }
}

@Override
public void run() {
    upCounter1();
    upCounter2();
    upCounter3();
}

public static void main(String[] args) {
    Threadtest mtt = new Threadtest();
    Thread t1 = new Thread(mtt);

    Thread t2 = new Thread(mtt);

    Thread t3 = new Thread(mtt);

    t1.start();
    t2.start();
    t3.start();
}

}

I tried this code with various synchronisation techniques and I'd like to make sure I get what's happening. I've read a bunch of articles on this, but none of them broke it down enough for me.

So here's what I observed:

1. synchronised (this): This works only, if I give the SAME instance of Threadtest to all threads, because if I give each thread its own instance, each will get that instance's intrinsic lock and can access the methods without interruption from the other threads.

2. However, if I give each thread its own instance, I can do: synchronised (getClass()), because then I get the instrinsic lock of the class

3. Alternatively, I could do: synchronised (mydate), where the same rules apply that apply to synchronised (this). But it has the advantage of not being public. > I dont really understand this. What is the "danger" of using this?

4. Alternatively to synchronised (getClass()), I could also use a private static field. However, I cannot do synchronised(Date.class).

5. I could synchronise the entire methods (same effecte as with synchronised-block)

6. making counter volatile doesn't work, because incrementing isn't a truly atomic operation

7. If I want to make each method accessible individually, I would make three private fields and use them in the synchronised-blocks. I then am effectively using the intrinsic locks of those fields and not of my class or instance.

8. I also noted that when I use the class-lock, each method is viewed as separate and I have effectively 3 ounters that go to 15. If I use the instance lock, the counter goes to 45. Is that the correct and expected behaviour?

Are my explanations and observations correct? (I basically want to make sure I draw the correct conclusions form the console output I got)

Answer 1

Your code is threadsafe as it stands, if slow (you are writing to the console while holding a lock) - but better correct and slow than wrong and fast!

a) synchronised (this): This works only, if I give the SAME instance of Threadtest to all threads, because if I give each thread its own instance, each will get that instance's intrinsic lock and can access the methods without interruption from the other threads.

Your code is threadsafe either case - that is, it will give the exact same results every time. If you pass the same instance to three different threads the final line of output will be "3 45" (since there is only one counter variable) and if you give each thread its own instance there will be three lines reading "3 15". It sounds to me like you understand this.

b) However, if I give each thread its own instance, I can do: synchronised (getClass()), because then I get the instrinsic lock of the class

If you do this your code is still threadsafe, but you will get three lines reading "3 15" as above. Be aware that you will also be more prone to liveness and deadlock issues for the reason stated below.

c) Alternatively, I could do: synchronised (mydate), where the same rules apply that apply to synchronised (this). But it has the advantage of not being public. I dont really understand this. What is the "danger" of using this?

You should try to use private locks where you can. If you use a globally-visible object (e.g. this or getClass or a field with visibility other than private or an interned String or an object that you got from a factory) then you open up the possibility that some other code will also try to lock on the object that you are locking on. You may end up waiting longer than you expect to acquire the lock (liveness issue) or even in a deadlock situation.

For a detailed analysis of things that can go wrong, see the secure coding guidelines for Java - but note that this is not just a security issue.

d) Alternatively to synchronised (getClass()), I could also use a private static field. However, I cannot do synchronised(Date.class).

A private static field is preferable to either getClass() or Date.class for the reasons stated above.

e) I could synchronise the entire methods (same effecte as with synchronised-block)

Pretty much (there are currently some insignificant byte code differences), but again you should prefer private locks.

f) making counter volatile doesn't work, because incrementing isn't a truly atomic operation

Yes, you may run into a race condition and your code is no longer threadsafe (although you don't have the visibility issue mentioned below)

g) If I want to make each method accessible individually, I would make three private fields and use them in the synchronised-blocks. I then am effectively using the intrinsic locks of those fields and not of my class or instance.

You should not do this, you should always use the same lock to access a variable. As well as the fact that you could have multiple threads reading/writing to the same variable at the same time giving race condition you also have a subtler issue to do with inter-thread visibility. The Java Memory Model guarantees that writes done by one thread before a lock is released will be seen another thread when that other thread acquires the same lock. So thread 2 executing upCounter2 may or may not see the results of thread 1 executing upCounter1.

Rather than thinking of "which blocks of code do I need to execute?" you should think "which pieces of state do I need to access?".

h) I also noted that when I use the class-lock, each method is viewed as separate and I have effectively 3 ounters that go to 15. If I use the instance lock, the counter goes to 45. Is that the correct and expected behaviour?

Yes, but it has nothing to do with the object you are using for synchronisation, rather it's because you have created three different ThreadTest objects and hence have three different counters, as I explained in my answer to your first question.

Make sure that you understand the difference between three threads operating on one object and one thread operating on three different objects. Then you will be able to understand the behaviour you are observing with three threads operating on three different objects.

Answer 2

a-c; e-f are correct.

c) Alternatively, I could do: synchronised (mydate), where the same rules apply that apply to synchronised (this). But it has the advantage of not being public. > I dont really understand this. What is the "danger" of using this?

The argument is that other code may also decide to use that object as a lock. Which could cause conflict; when you know that this can never be the case then it is not such an evil thing. It is also usually more of a problem when one uses wait/notify in their code.

d) Alternatively to synchronised (getClass()), I could also use a private static field. However, I cannot do synchronised(Date.class).

You can use Date.class, it would just be a bit weird and falls into the argument discussed in c above about not polluting other classes work spaces.

g) If I want to make each method accessible individually, I would make three private fields and use them in the synchronised-blocks. I then am effectively using the intrinsic locks of those fields and not of my class or instance.

Given that the three methods share the same state, then no, this would not be wise as it would lead to races between the threads.

h) I also noted that when I use the class-lock, each method is viewed as separate and I have effectively 3 counters that go to 15. If I use the instance lock, the counter goes to 45. Is that the correct and expected behaviour?

No, this sounds wrong but I may have misunderstood you. I would expect the total to be 45 in both cases when using either this or this.getClass() as the lock.

Answer 3

a) Correct b) Correct c) There could be some other bunch of code using your this or class in another part of your application where your class is accessible. This will mean that unrelated code will be waiting for each other to complete. d) You cannot do synchronisation on Date.class because of the same reason above. There may be unrelated threaded methods waiting for each other unnecessarily. e) Method synchronisation is same as class lock
g) Correct