CODEWITHSUNDEEP
javamultithreadingsynchronized
Hele
Hele

Reputation: 1578

Java synchronized block unexpected behavior

I have the following code : 

public class Experimenter implements Runnable
{
    private volatile Integer a = new Integer(0);

    public Experimenter() throws Exception
    {
        System.out.println("start");
    }

    public void funk() throws InterruptedException
    {
        synchronized (a)
        {
            System.out.println("in");
            Thread.sleep(5000);
            System.out.println("out");
        }
    }

    public static void main(String[] args) throws Exception
    {
        Thread a = new Thread(new Experimenter(), "a");
        Thread b = new Thread(new Experimenter(), "b");
        Thread c = new Thread(new Experimenter(), "c");
        Thread d = new Thread(new Experimenter(), "d");
        Thread e = new Thread(new Experimenter(), "e");

        a.start();
        b.start();
        c.start();
        d.start();
        e.start();
    }

    @Override
    public void run()
    {
        try
        {
            funk();
        } 
        catch (InterruptedException e)
        {
            e.printStackTrace();
        }
    }
}

I had expected since only one thread could use the synchronized block at a time, it would print something like the following, with a 5 second gap between each in and out :

start
start
start
start
start
in
out
in
out
in
out
in
out
in
out

But instead, I get the following. All the ins, 5 seconds later, all the outs.:

start
start
start
start
start
in
in
in
in
in
out
out
out
out
out

Could someone help explain this?

Upvotes: 2

Views: 70

Answers (4)

JB Nizet
JB Nizet

Reputation: 692131

Each thread synchronizes on its own lock, since each thread uses its own Experimenter instance, and each Experimenter instance has its own Integer instance serving as a lock. If you want the threads to synchronize, you need to share a unique lock between all instances. You shouldn't use an Integer, BTW. Use a simple Object:

final Object sharedLock = new Object();
Thread a = new Thread(new Experimenter(sharedLock), "a");
Thread b = new Thread(new Experimenter(sharedLock), "b");
...

The use of the final keyword : It's not strictly needed. But locks, especially when they're declared as fields, should be final to make sure you don't accidently assign a new value to the variable, which would cause threads to use two different lock objects

Upvotes: 2

fge
fge

Reputation: 121820

Quite simple: your a is not shared between any of your Experimenters; it is an instance variable, one per Experimenter. And the volatile is pretty much useless in this situation by the way.

If you want a shared lock, make it a private static final variable. And no need for volatile in this situation either!

But I would go with @JBNizet's solution which is much cleaner.

EDIT: why final? Because it is guaranteed to never change once initialized; but the most important aspect of a final variable comes from the Java Memory Model, which states that the initialization of a final variable happens-before any read of this variable. Which is a very powerful rule.

Upvotes: 3

kmera
kmera

Reputation: 1745

Declare the field a as static and it will work as expected. In your code the field is an instance member, so you have in fact five different monitors.

Upvotes: 1

Bhargav Modi
Bhargav Modi

Reputation: 2655

this will surely work for you

public void funk() throws InterruptedException
{
    synchronized (Experimenter.class)
    {
        System.out.println("in");
        Thread.sleep(5000);
        System.out.println("out");
    }
}

Upvotes: 1

Related Questions