Why is this code not 'thread-safe'?

Question

class ThreadUnsafe 
{ 
    static int  val1,  val2; 
    static void Go()
    { 
        if  (val2 != 0)  System.out.println( val1 / val2); 
        val2=0; 
    } 
}

Apparently the Go() method in this code is not considered "thread-safe". Why is this?

Because `val2` can change (by another thread) between `val2 != 0` and `val1 / val2`. I'm not *entirely* familiar with the thread capabilities and conventions in Java, but what I'd probably do here is capture `val2` into a method-local variable and then perform the logic on that variable. — David, Apr 24 '18 at 16:13
There is also not synchronization for access to `val1` and `val2`. So first thread may not even know that one of the variables was changed in another thread. — Ivan, Apr 24 '18 at 16:19
If you are not updating val1 & val2 outside Go() method then you are unlikely to see any issues with multi-threading — nitnamby, Apr 24 '18 at 16:20

Sometimes_Confused · Answer 1 · 2018-04-24T17:24:36.907

Normally in Java when concurrency is expected to be an issue either the synchronized keyword would be used on methods that change or read the internal state of the object or a synchronized block would be used with a lock on a non-null object. Both would require that accessor methods be created for setting the value of val1 and val2.

Example using synchronized on the methods that change (or read) internal state:

class ThreadSafe
{
    private static int val1;
    private static int val2;

    static synchronized void go()
    {
        if (val2 != 0)
        {
            System.out.println(val1 / val2);
        }

        val2 = 0;
    }

    static synchronized void setVal1(int newVal1)
    {
        val1 = newVal1;
    }

    static synchronized void setVal2(int newVal2)
    {
        val2 = newVal2;
    }
}

In this case because the methods are static, the synchronization will occur on the class itself.

Example using synchronization on a non-null object:

class ThreadSafe
{
    private static int val1;
    private static int val2;

    private static Object lock = new Object();

    static void go()
    {
        synchronized (lock)
        {
            if (val2 != 0)
            {
                System.out.println(val1 / val2);
            }

            val2 = 0;
        }
    }

    static void setVal1(int newVal1)
    {
        synchronized (lock)
        {
            val1 = newVal1;
        }
    }

    static synchronized void setVal2(int newVal2)
    {
        synchronized (lock)
        {
            val2 = newVal2;
        }
    }
}

Why is this code not 'thread-safe'?

1 Answers1