Multi-Thread Producer/Consumer Synchronization Issues

Question

I am having issues with two threads that dont seem to synchronize properly. I basically have an boolean value names "occupied". It is set to false when no thread is started. but when one starts, the thread sets occupied is true I have a class that has the thread (run) and they call the functions below.

This is a mock bank example that takes in an amount (initial balance) and then performs withdrawals and deposits randomly. My professor mention something about signaling form the withdraw thread to the deposit thread? How does that work? The withdraw thread, it should run until the balance is two low and wait for a deposit thread. How should i do that?

   package bank;

import java.util.Random;
import bank.Bank;
import java.util.concurrent.locks.ReentrantLock;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.Condition;

/**
 *
 * @author KJ4CC
 */
public class Action {

    private Lock accessLock = new ReentrantLock();
    private Condition cond = accessLock.newCondition();
    //private Condition withdraw  = accessLock.newCondition();


    Random rand = new Random();
    Object lock = new Object();
    Bank getBalance = new Bank();

    public void widthdrawl(int threadNum) throws InterruptedException {
        int amount = rand.nextInt(50);
            accessLock.lock();

            if (getBalance.getbalance() > amount) {

                getBalance.setBalance(getBalance.getbalance() - amount);

                System.out.println("\t\t\tThread " + threadNum + " withdrawls " + amount + "\t Balance is " + getBalance.getbalance());

            } else {

                System.out.println("\t\t\tThread " + threadNum + " Failed to withdrawl " + amount + "\t Balance is " + getBalance.getbalance());
                cond.await();

            }


            accessLock.unlock();
            Thread.sleep(rand.nextInt(5));

    }

    public void deposit(int threadNum) throws InterruptedException {
        int amount = rand.nextInt(200);
        accessLock.lock();


            getBalance.setBalance(getBalance.getbalance() + amount);
            System.out.println("Thread " + threadNum + " Deposits " + amount + "\t\t\t\t Balance is " + getBalance.getbalance());
            Thread.sleep(rand.nextInt(100));

            cond.signal();
            accessLock.unlock();


    }
}

Try checkout out a BlockingQueue. – Leo Izen Sep 21 '16 at 15:45 — Leo Izen, Sep 21 '16 at 15:45

score 4 · Answer 1 · answered Sep 21 '16 at 15:35

4

Your Lock and Condition usage is wrong. You're calling lock() without calling unlock() anywhere, and you're calling signal() without anyone having called await().

See the documentation for an example very much related to your problem.

answered Sep 21 '16 at 15:35

Kayaman

72,141
5
83
121

Andriy Kryvtsun · Answer 2 · 2016-09-21T15:47:06.283

0

First, you have to mark you occupid variable volatile. Without this keyword changing the variable value in one thread won't be visible in another.

Second, you trying to implement external synchronization policy for bank entity. Such idea basically isn't good: if someone uses the same bank without properly synchronization it'll break internal bank state. It's better to implement internal synchronization policy and allow bank to guard its state by itself.

E.g. the API of Bank class may be like this

class Bank {
   double synchronize getBalance() { ... }
   void synchronize deposit(double amount) { ... }
   void synchronize widthdrawl(double amount) throw BankException { ... }
}

With this design internal Bank state always be consistent and any Bank users will be wait finishing current bank operation automatically.

edited Sep 21 '16 at 15:47

answered Sep 21 '16 at 15:17

Andriy Kryvtsun

3,220
3
27
41

Thanks for the tip. – Johnathan Yaesu Sep 21 '16 at 15:28
1

He doesn't need `synchronized`, he's using `Locks`. He's just using them wrong. – Kayaman Sep 21 '16 at 15:46
@Kayaman Actually, he doesn't need any locks in Action class. The whole design isn't good what I'm saying and proposed better and simpler design – Andriy Kryvtsun Sep 21 '16 at 15:49
@AndriyKryvtsun I know what you're saying, but you're proposing (hopefully) a `synchronized/wait()/notify()` solution instead of a more modern `lock/await()/signal()`. – Kayaman Sep 21 '16 at 15:52
Per the assignment i can not use the synchronize block – Johnathan Yaesu Sep 21 '16 at 15:54
@Kayaman No. I'm proposing don't do external synch at all. I'm proposing doing internal synch. In this case you don't need and wait/notify in your client code. – Andriy Kryvtsun Sep 21 '16 at 15:54
1

@AndriyKryvtsun His question even states: "it should run until the balance is two low and **wait** for a deposit thread.". So yes, he's going to need wait/notify functionality. You're right that it's not smart design to have the locking in an `Action` class, but you're wrong about his actual problem. – Kayaman Sep 21 '16 at 15:58
@Johnathan I see. So remove 'synchrinized' from my example and put your 'accessLock' instance inside Bank class. With this you'll have guided bank instance and no external synch. – Andriy Kryvtsun Sep 21 '16 at 16:01

Multi-Thread Producer/Consumer Synchronization Issues

2 Answers2