2

In my program I have a class called "Vehicle" which extends from Thread, the thing is that at a certain point I need these Threads to create a Task (Runnable) and I want all these tasks to be managed by the same Threadpool, the problem is that if I call a method from a different class that contains this Pool, every other thread is creating a different pool. How can I avoid this? Thanks in advance.

public class Station {
    Arrivals arrivals;
    Vehicle k;
    ListPumps lp;

    ExecutorService executor = Executors.newFixedThreadPool(3);
    public synchronized void startWork(Pump p) {

        Runnable w = new Work(p);
        executor.execute(w);

        executor.shutdown();
        while (!executor.isTerminated()) {
    }
        System.out.println("Finished all threads");
     }
}
Vasil Lukach
  • 3,658
  • 3
  • 31
  • 40
J.FF
  • 41
  • 4

2 Answers2

2

Your code already uses a single threadpool. However if your class might have several instances and you want those to all share the same threadpool then assign it as a static variable.

Though I would also pull out the shutdown code to its own method too now. Also don't poll isTerminated, use awaitTermination.

Something like the following:

public class Station {

    private Arrivals arrivals;
    private Vehicle k;
    private ListPumps lp;

    private static ExecutorService executor = Executors.newFixedThreadPool(3);

    public void startWork(Pump p) {
        Runnable w = new Work(p);
        executor.execute(w);
    }

    public static synchronized void shutdown() {
        executor.shutdown();
        if(executor.awaitTermination(60, TimeUnit.SECONDS))
            System.out.println("Finished all threads");
        else
            System.out.println("Executor shutdown timed out");
    }
}
Jeffrey Phillips Freeman
  • 897
  • 1
  • 9
  • 28
  • this can be part of comment for this question. This did not answer the question. – Vishrant Apr 28 '18 at 18:58
  • 1
    But just for curiosity, is it required to make `startWork` as synchronized? Because `ExecutorService` will execute new task with available thread and if thread is not free then the task will wait? – Vishrant Apr 28 '18 at 20:03
  • 1
    @Vishrant I just double checked and you are correct, the execute method is in fact thread safe, however shutdown doesnt appear to be (I may be wrong here). I will modify my example accordingly. – Jeffrey Phillips Freeman Apr 28 '18 at 20:06
  • It seems to me shutdown is also threadsafe. Documentation says `Initiates an orderly shutdown in which previously submitted tasks are executed` however `This method does not wait for previously submitted tasks to complete execution` – Vishrant Apr 28 '18 at 20:16
  • but I guess in this perticular case (class `Station`) its better to have a class level lock with static synchronized. – Vishrant Apr 28 '18 at 20:18
  • 1
    Vishrant I am not sure that excerpt actually states it is thread-safe. As in, it is safe to call shutdown simultaneously from multiple threads. I strongly suspect it is thread-safe but not explicitly mentioned as such int he javadocs. I simply cant find any garuntees to this regard. – Jeffrey Phillips Freeman Apr 28 '18 at 20:19
  • When you call `Executors.newFixedThreadPool` it creates an object of `ThreadPoolExecutor` and I confirmed that all the methods of `ThreadPoolExecutor` are indeed thread safe (it uses `ReentrantLock` they call it as mainlock in source code). Here is a discussion https://stackoverflow.com/questions/1702386/is-threadpoolexecutor-thread-safe#answer-7596354 – Vishrant Apr 28 '18 at 20:28
  • "It is fine to construct and submit jobs to any (correctly implemented) ExecutorService without external synchronisation. This is one of the main design goals." – Vishrant Apr 28 '18 at 20:30
  • @Vishrant yea i read that as well. He made a good enough argument for submit/execute which is why i removed the synchronize. But as he goes on to explain in that ticket for termination the wording isnt as clear, though it is implied. I would pesonally remove the synchronized call in my own code. Just reluctant to leave it out on SO without some formal justification... I can go either way on this. If you wish to edit my question to remove it I wont object, your call. – Jeffrey Phillips Freeman Apr 28 '18 at 20:39
  • sure :) I am just being more picky about using synchronized keyword. ;) Anyways I updated my answer with `singleton` implementation – Vishrant Apr 28 '18 at 21:01
  • BTW you cannot use final with volatile, thanks for suggesting an edit for my post. – Vishrant Apr 28 '18 at 21:06
  • @Vishrant Of course you can declare variables static and final: https://www.geeksforgeeks.org/final-static-variable-java/ – Jeffrey Phillips Freeman Apr 28 '18 at 21:07
  • Oh! `final` with `volatile` is not possible, my bad. I used volatile in my implementation. – Vishrant Apr 28 '18 at 21:08
0

Two options:

  1. You can declare ExecutorService object as static, or:
  2. Create Station class as singleton

I recommend reading this post: should ExecutorService be static and global

public class Station {

    private Arrivals arrivals;
    private Vehicle k;
    private ListPumps lp;

    // Bill Pugh Singleton Implementation
    private static class ExecutorServiceHelper {

        // volatile keyword to avoid CPU caching EXECUTOR object
        // famous volatile illutration http://tutorials.jenkov.com/images/java-concurrency/java-volatile-2.png
        private static volatile ExecutorService EXECUTOR = Executors
                .newFixedThreadPool(3);;

    }

    public ExecutorService getInstance() {
        return ExecutorServiceHelper.EXECUTOR;
    }

    // implementation of executor framework is threadsafe, so you don't need 
    // to use synchronized keyword on methods calling executors methods
    // here is the discussion https://stackoverflow.com/questions/1702386/is-threadpoolexecutor-thread-safe#answer-7596354
    public void submitWork(Pump p) {
        Runnable w = new Work(p);
        getInstance().execute(w);
    }

    public void shutdown() {
        getInstance().shutdown();
        if(getInstance().awaitTermination(60, TimeUnit.SECONDS))
            System.out.println("Finished all threads");
        else
            System.out.println("Executor shutdown timed out");
    }


}
Vishrant
  • 15,456
  • 11
  • 71
  • 120
  • Your recent edits removed my own edits. Your code as-is wont compile. There are atleast three bugs that I had fixed that you removed with your edit. – Jeffrey Phillips Freeman Apr 28 '18 at 21:11
  • 1
    Wo! I did not realize that. But what are the three bugs? @JeffreyPhillipsFreeman – Vishrant Apr 28 '18 at 22:25
  • Sorry 5 bugs. I just submitted an edit with them fixed, you can see them int he diff. – Jeffrey Phillips Freeman Apr 28 '18 at 22:28
  • 1
    Thanks for finding out, I agree there is 1 bug. Which is in `shutdown` method I should use `getInstance()` instead of `executor` but others are correct and the code will compile. Though I can make `getInstance` method static as J.FF is creating the object of `Station` class he will get an instance to call `getInstance` which will lazily load `ExecutorServiceHelper` class. Its just another way of writing it. :) I will accept your one edit. @JeffreyPhillipsFreeman – Vishrant Apr 28 '18 at 23:29
  • no the others wont compile for a slew of reasons. In your code you also had EXECUTOR as private so when get instance was outside of that class it couldnt access it and would fail. So I moved it to be inside the singleton class. That however now meant you have to refer to it by the class specifier, thus the other bugs. You could of course have made it public, but then of course it is setable so itsnt a true singleton and the code therefore adds nothing of value anyway. Either way you look at it there was more than one bug though. – Jeffrey Phillips Freeman Apr 28 '18 at 23:33
  • 1
    You can access the private variables within the same class. https://imgur.com/HJ5AkMe this what I just wrote in eclipse. This is the pastebin for what image I pasted https://pastebin.com/ciR0Xae7 @JeffreyPhillipsFreeman – Vishrant Apr 28 '18 at 23:38