java method prevent from concurrent access

Question

How can I prevent from concurrent access. I have code like this

public class MC implements Runnable {

    public void run() {
        sync();
    }

    public static void main(String p[]){
        MC mc = new MC();
        MC mc2 = new MC();
        MC mc3 = new MC();
        MC mc4 = new MC();
        Thread t = new Thread(mc);
            t.start();
            Thread t2 = new Thread(mc2);
            t2.start();
            Thread t3 = new Thread(mc3);
            t3.start();
            Thread t4 = new Thread(mc4);
            t4.start();
    }

    private synchronized void sync(){
        try {
            System.out.println(System.currentTimeMillis());
            Thread.sleep(10000);
        } catch (InterruptedException ex) {
            ex.printStackTrace();
        }
    }
}

and I am getting output like this

1307082622317
1307082622317
1307082622317
1307082622317
BUILD SUCCESSFUL (total time: 11 seconds)

any advice?

Answer 1

make your method static:

private static synchronized void sync();

your method as coded is synchronized on the instance, but each thread has its own instance, so there's no synchronization.

static methods are synchronized on the Class object, of which there is only one per class, so all instances will synchronize on static methods.

Answer 2

You've got four separate MC objects. Typically running an instance method on those (sync), they shouldn't interfere with each other. You can use a synchronized block to make sure only one runs at a time, but you need to consider what to synchronize on:

• If you synchronize on a separate object per instance, that would stop two threads from running the code for the same object. That's effectively what you've got now, but you're implicitly synchronizing on this, which I would discourage you from doing. (Any other code could synchronize on the same object.)
• If you synchronize on an object that all the instances know about (e.g. via a static variable) then that would only let one thread run the code at all.

It sounds like you want the latter approach, but it doesn't sound like great design to me. If you really want to implement it that way, you'd use:

public class MC implements Runnable {

    private static readonly Object lock = new Object();

    ...

    private void sync() {
        synchronized (lock) {
            try {
                System.out.println(System.currentTimeMillis());
                Thread.sleep(10000);
            } catch (InterruptedException ex) {
                ex.printStackTrace();
            }
        }
    }
}

Keeping sync as a synchronized method but making it static would also work, but again you'd be locking on a publicly visible object (MC.class) which I generally discourage.