Semaphore - why do my threads work one after another, and not at the same time?

Question

I am trying to write a program that in the Main class can initiate an unknown number of new threads. Each thread, in turn, must call the Singleton Copier class, which must trigger the file transfer action.

My goal, regardless of the number of thread requests, is to limit the number of simultaneous transfers in 2 transfers, so I decided to solve it with Semaphore . My problem is that threads work one by one, and not in parallel.

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); } } 

At startup - you can see by the output streams running one after the other, while my goal is to have up to two simultaneous streams. What have I done 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 should call start() instead of run() , otherwise it will not start your threads, so the transfer will be executed sequentially, which is actually the main cause of your current problem.

In any case, for me, your code should be rewritten, since the Copier class should even start () threads, since this is not its responsibility.

1. Rewrite the fileTransfer() method

 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. Deploy the run() CopyThread correctly

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

3. Make the semaphore non-static and final

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

4. Use the inner class to lazily 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.. 

Answer 2

If you call Thread.run() , you do not start the thread, you simply execute the method sequentially. You need to call start() . (I assume CopyThread is Thread ).

Joshua Bloch's Java Puzzlers have a chapter with a very similar example.