Semaphore - why my threads are running one after the other and not in concurrent?

Question

I'm trying to write a program that in the Main class one can initiate unknown amount of new threads. Each thread in turn should call to a Singleton Copier class which should call a file transfer action.

My goal is, regardless the amount of threads requests, is to limit the number of concurrent transfers to 2 transfers, so I thought solving it with Semaphore. My problem is, it's seems the threads are running one after the other and not in concurrent.

Here is what I tried to do:

public class Copier {

    private static final int POOL_SIZE = 2;
    private static volatile Copier instance = null;
    private static Semaphore semaphore;

    private Copier() {

    }

    public static Copier getInstance() {
        if (instance == null) {
            synchronized (Copier.class) {
                if (instance == null) {
                    instance = new Copier();
                    semaphore = new Semaphore(POOL_SIZE);
                }
            }
        }
        return instance;
    }

    public void fileTransfer(CopyThread copyThread) {
        try {
            semaphore.acquire();
            System.out.println("Running thread...");
            copyThread.run();

        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        finally {
            semaphore.release();
            System.out.println("Thread released..");
        }
    }
}

This is my Main class:

public class Driver {
    public static void main(String[] args) {
        Copier copier = Copier.getInstance();
        CopyThread copyThread1 = new CopyThread();
        CopyThread copyThread2 = new CopyThread();

        copier.fileTransfer(copyThread1);
        copier.fileTransfer(copyThread2);
    }
}

When running it - you can see by the output the threads running one after the other, while my purpose is to have up to 2 concurrent threads. What did I do wrong?

Running thread...
3.998784MB were transferred in 5.902514932 seconds
Thread released..
Running thread...
4.062673MB were transferred in 7.199550077 seconds
Thread released..

Answer 1

You are supposed to call start() instead of run() otherwise it won't start your threads such that the transfer will be done sequentially which is actually the root cause of your current issue.

Anyway, for me your code should be rewritten as the class Copier should not even start() the threads as it is not its duty.

1. Rewrite the method fileTransfer()

public void fileTransfer() {
    try {
        semaphore.acquire();
        System.out.println("Running transfer...");
        // Code that performs the transfer
    } catch (InterruptedException e) {
        e.printStackTrace();
    } finally {
        semaphore.release();
        System.out.println("Thread released..");
    }
}

2. Implement the method run() of CopyThread properly

@Override
public void run() {
    // Here I call fileTransfer() on Copier instead of the other way around
    Copier.getInstance().fileTransfer();
}

3. Make semaphore non static and final

private final Semaphore semaphore;

private Copier() {
    this.semaphore = new Semaphore(POOL_SIZE);
}

4. Use an inner class to lazy create your instance

public class Copier {
    ...
    public static Copier getInstance() {
        return Holder.instance;
    }
    ...
    private static class Holder {
        private static final Copier instance = new Copier();
    }
}

5. Rewrite your main method

public static void main(String[] args) throws Exception {
    CopyThread copyThread1 = new CopyThread();
    CopyThread copyThread2 = new CopyThread();

    copyThread1.start();
    copyThread2.start();
}

Output:

Running transfer...
Running transfer...
Thread released..
Thread released..