Java synchronized confusion

Question

sorry if this is extremely obvious or has been answered elsewhere. I haven't been able to find anything. I have the following code:

public class SimpleThread extends Thread {
    public static Integer sharedVal = 0;
    public SimpleThread() {

    }
    @Override
    public void run() {
       while(true) {           
           iterator();
       }
    }

    public void theSleeper() {
        System.out.println("Thread: " + this.getId() + " is going to sleep!");
        try {
            this.sleep(5000);
        } catch(Exception e) {

        }
    }

    public void iterator() {
        synchronized(sharedVal)  {
            System.out.println("Iterating sharedVal in thread: " + this.getId());
            sharedVal++;
            System.out.println(sharedVal);
            theSleeper();
            System.out.println("Thread : " + getId() + " is done sleeping, trying to iterate again..."); 
        }
    }
}

I create two instances of this SimpleThread class, and execute the run methods. I would expect to see something like: Thread 9 incrementing... Thread 9 sleeping... (after 5 seconds) Thread 10 incrementing.... Thread 10 sleeping... . I expect this because I am locking the iterator method so that only one thread at a time should be able to enter it. What happens instead is that both threads increment, and then both wait 5 seconds. This repeats forever. What am I missing here such that I would get the expected behavior? Thanks very much!

EDIT: I made a new public static variable: public static Object theLock = new Object(). Now, in the iterator method, I do synchronized(theLock). The output is now more as I expected, because the lock never changes. However, now only thread 9 ever enters the method. It seems that thread 10 is starving, and never gets a turn. This seems strange. It's not just a few times, its always just thread 9 iterating and sleeping. I would think that it would be 9, 10, 9, 10. Or perhaps a random distribution like 9, 10, 10, 10, 9, 10, 9, 9, 10, etc.

EDIT2: I see what's happening now. Thread 9 has the lock. Thread 10 tries to enter the function, but is immedietly told to wait. After the function completes, it may still be Thread 9s turn. Thread 9 then reaqquires the lock, and the cycle continues. The timing window for thread 10 to get a turn is very small, and if it did get a turn it would likely starve 9. That being said, putting yield() after the synchronized block in iterator() does not seem to make it more fair. I read the comments on the method, and the scheduler can in fact ignore yield().

Hahaha, it seems it can be problematic! :) – user2045279 Nov 04 '13 at 19:31 — user2045279, Nov 04 '13 at 19:31

score 5 · Answer 1 · answered Nov 04 '13 at 19:01

5

When you are doing increment you create a new instance of integer and a new object for locking.

answered Nov 04 '13 at 19:01

anstarovoyt

7,956
2
27
35

1

@OP: Try it again using a something that is mutable, like a StringBuilder (NOT StringBuffer since StringBuffer is synchronized.) – MadConan Nov 04 '13 at 19:03
I completely agree, and I feel dumb. I don't know why Integer is immutable, but that's another issue. Please see my edit. – user2045279 Nov 04 '13 at 19:08
theLock.wait(5000); does not wait at all, in fact. I tried LOCK and lock but I think you were referring to whatever my lock was. This does, however, alternate 9 and 10. Could just be a wierd timing issue with the sleep, though I would like to understand why 10 is getting starved – user2045279 Nov 04 '13 at 19:15
Oh really? That's good to know actually... But when it exits the synchronized block it should get released, right? Like in my code, iterator() has a synchronized block, and the sleep function call is within it. – user2045279 Nov 04 '13 at 19:16

score 3 · Accepted Answer · answered Nov 04 '13 at 19:03

3

Your problem resides here:

sharedVal++;

That line, due to autoboxing translates into this:

sharedVal = Integer.valueOf(sharedVal.intValue() + 1);

which creates a new Integer object every time it runs, so every time the synchronized block isreached, it locks a different value.

Use a dedicated object to synchronize:

private final static Object LOCK = new Object();

and then change your synchronization to use synchronized (LOCK) {... instead.

(You could also use getClass() to lock, but I personally don't like to expose lock objects to the public world)

answered Nov 04 '13 at 19:03

Darkhogg

13,707
4
22
24

When thread 9 comes back from the sleep, it releases the lock and then re-acquires it, leaving thread 10 with no possibilities. The only chance it has is the scheduler stopping thread 9 betweet locks, which is unlikely. Add a `Thread.yield()` between iterations, that will make things a little bit more fair. – Darkhogg Nov 04 '13 at 19:16
Yes, I see that now and added this to my second edit. Thread.yield() doesn't seem to help much because the scheduler is probably ignoring it. That being said, I think I understand a lot more now. Thanks very much! – user2045279 Nov 04 '13 at 19:26

score 1 · Answer 3 · answered Nov 04 '13 at 19:01

1

You are effectively changing the lock that threads are using on every iteration: ++ on Integer creates a new instance (Integer class is immutable)

answered Nov 04 '13 at 19:01

Sami Korhonen

1,254
8
17

Yes, another mentioned that as well, thanks for pointing out this error. Please see my edit – user2045279 Nov 04 '13 at 19:08

score 0 · Answer 4 · answered Nov 04 '13 at 19:02

0

sharedVal member varible instance is changing when you perform sharedVal++. Instead you can use for synchronization the following statement:

synchronized(SympleThread.class)

answered Nov 04 '13 at 19:02

Nikoloz

453
1
3
11

Java synchronized confusion

4 Answers4