Java Thread safety - Understanding the need of synchronization

Question

I am having a very hard time trying to understand the concept of synchronizing methods, objects and understand the main issue of not doing so, when running a multi-threaded application.

I understand that synchronize keyword is used to make sure that only one thread will work with a specific object or enter a specific block or method in a time, basically locks it and unlocks when the execution ended, so the other threads can enter it.

But I don't really understand the problem, I am totally confused, I created a demo application, where I have 2 bank accounts, and one bank class which has 5000 funds and a method that transfers a specific amount of money to the given account, and in it's constructor it creates the 2 bank accounts and start the threads (each account is a thread).

Now in the bank account's class I have a funds field, and a run method which the thread will call upon start (the class inheriting Thread), and the run method will loop 10 times, and take 20 dollar from the main bank by calling Bank#takeFunds(int amount)

So there we go, the Bank class:

public class Bank {

    private int bankmoney = 5000;

    public Bank() {
        Client a = new Client(this);
        Client b = new Client(this);

        a.start();
        b.start();
    }

    public void takeMoney(Client c, int amount) {
        if (bankmoney >= amount) {
            bankmoney -= amount;
            c.addFunds(amount);
        }
    }

    public void print() {
        System.out.println("left: " + bankmoney);
    }

    public static void main(String... args) {
        new Bank();
    }
}

And the Client class:

public class Client extends Thread {

    private Bank b;
    private int funds;
    Random r = new Random();
    public Client(Bank b) {
        this.b = b;
    }

    public void addFunds(int funds) {
        this.funds += funds;
    }

    public void run() {
        for (int i = 0; i < 10; i++) {
            b.takeMoney(this, 20);
        }
        System.out.println(Thread.currentThread().getName() + " : " + funds);
        b.print();
    }
}

And the output for me:

Thread-0 : 200
left: 4800
Thread-1 : 200
left: 4600

The program ends with 200$ in each account, and 4600 left in the bank, so I don't really see the issue, I am failing to demonstrate the issue of thread safety, and I think this is why I can't understand it.

I am trying to get the most simple explanation on how it works exactly, How can my code turn into a problem with thread safety?

Thanks!

Example:

static void transfer(Client c, Client c1, int amount) {
    c.addFunds(-amount);
    c1.addFunds(amount);
}

public static void main(String... args) {
    final Client[] clients = new Client[]{new Client(), new Client()};

    ExecutorService s = Executors.newFixedThreadPool(15);
    for (int i = 0; i < 15; i++) {
        s.submit(new Runnable() {
            @Override
            public void run() {
                transfer(clients[0], clients[1], 200);
            }
        });
    }

    s.shutdown();
    while(!s.isTerminated()) {
        Thread.yield();
    }

    for (Client c : clients) {
        c.printFunds();
    }
}

Prints:

My funds: 2000
My funds: 8000

Answer 1

I tested your program, and in fact got the following output:

(The result is not 4600 as in your case.)

The point is that just because it happens to work once doesn't mean that it will always work. Multi threading can (in an illsynchronized program) introduce non-determinism.

Imagine what would happen if your operations took a bit longer to execute. Let's simulate this with a Thread.sleep:

public void takeMoney(Client c, int amount) {
    if (bankmoney >= amount) {
        try { Thread.sleep(1000); } catch (InterruptedException e) { }
        bankmoney -= amount;
        c.addFunds(amount);
    }
}

Now try running your program again.

Answer 2

Any time there's something you want to do to data that are shared by more than one thread, if it takes more than one step, then you probably need synchronization.

This takes three steps:

i++;

The steps are; (1) get the value of i from memory into a register, (2) add 1 to the register, (3) store the value of the register back into memory.

A running thread can be preempted at any time. That means, the operating system can pause it, and give some other thread a turn using the CPU. So, if there's no synchronization, thread A could perform step (1) of incrementing i (it could get the value into a register), and then it could be preempted. While thread A is waiting to run again, threads B, C, and D could each increment i a thousand times. Then when thread A finally got to run again, it would add 1 to the value that it originally read, and then store that back into memory. The three thousand increments by threads B, C, and D would be lost.

You need synchronization whenever one thread could put some data into a temporary state that you don't want other threads to see or operate on. The code that creates the temporary state must be synchronized, and any other code that could operate on the same data must be synchronized, and any code that merely allows a thread to see the state must synchronized.

As Marko Topolnik pointed out, synchronization doesn't operate on data, and it doesn't operate on methods. You need to make sure that all of the code that modifies or looks at a particular collection of data is synchronized on the same object. That's because synchronization does one thing, and one thing only:

The JVM will not allow two threads to be synchronized on the same object at the same time. That's all it does. How you use that is up to you.

If your data are in a container, it may be convenient for you to synchronize on the container object.

If your data are all instance variables of the same Foobar instance, then it may be convenient for you to synchronize on the instance.

If your data are all static, then you probably should synchronize on some static object.

Good luck, and have fun.

Answer 3

Let's look at a more realistic example and implement a transfer function for our Bank:

public boolean transfer(long amount, Client source, Client recipient) {
    if(!source.mayTransferAmount(amount)) return false; // left as an exercise
    source.balance -= amount;
    recipient.balance += amount;
}

Now let's imagine two threads. Thread A transfers a single unit from Client x to Client y while Thread B transfers a single unit from Client y to Client x. Now you must know that without synchronization, you cannot be sure how the CPU orders operations, so it could be:

A: get x.balance (=100) to tmpXBalance
B: get x.balance (=100) to tmpXBalance
B: increment tmpXBalance (=101)
B: store tmpXBalance to x.balance (=101)
A: decrement tmpXBalance (=99)
A: store tmpXBalance to x.balance (=99)
(rest of exchange omitted for brevity)

Whoa! We just lost money! Client x won't be very happy. Note that locking alone won't give you any guarantee, you also need to declare balance as volatile.

Answer 4

your program is working fine , as you are only deducting total amount of 2000. Which is far lesser than initial value. So, this check has no play, you code will work even if you rmeove it.

 if (bankmoney >= amount) {

The only bad thing that can happen in this scenario , if client1 checks that amount is more than he needs to withdraw , but in meantime other client withdraws it.

public void run() {
    for (int i = 0; i < 100; i++) {
        b.takeMoney(this, 200);
    }
    System.out.println(Thread.currentThread().getName() + " : " + funds);
    b.print();
}


 public void takeMoney(Client c, int amount) {
    if (bankmoney >= amount) {
    system.println("it is safer to withdraw as i have sufficient balance")
        bankmoney -= amount;
        c.addFunds(amount);
    }
}

there will be time when client one will check bankmoney is greater than amount , but when he withdraws, it will reach to negative amount. as other thread will take that amount. Run program, 4-5 times you will realize.

Answer 5

To start with, a thread is not an object. Do not assign a separate thread to each client. Threads do work and objects contain code which specifies what must be done.

When you call methods on a Client object, they do not execute "on that client's thread"; they execute in the thread from which they are called.

In order to make a thread do some work, you need to hand it over an object implementing the code to be executed on it. That's what an ExecutorService allows you to do simply.

Also keep in mind that locks do not "lock objects" and synchronized(anObject) will not on its own stop another thread from calling anObject's methods at the same time. Locks only prevent other threads trying to acquire the same lock from proceeding until the first thread is done with it.