1

There is an utility class called "ConcurrencyUtils" that uses ExecutorService internally

private static final ExecutorService executor = Executors.newCachedThreadPool(new CommonPoolThreadFactory());

as you can see it uses a thread pool that will add threads on demand. The problem is that if there is at least one thread in that pool (if util is used at least once) then when main method execution finishes application however didnt shut down, as the threads are still alive and are not garbage collected...

initially i've tried to use 

public static void close(){
    executor.shutdown();
}

and call it when the programm naturally should stop its execution. But there is no guarantee that main method is the last one to be finished and there can be other tasks that should finish before application will be terminated thus I dont know where do i call "close()" to clear up threads in util...

I wonder if community can hint me a smarter/cleaner way to implement this?

ConcurrencyUtil is intended to be available for use from everywhere and I cannot create an instance of it I need it to provide its interface through static not instance methods.

Update

Let me explain this case in detail, ConcurrencyUtil has its own implementation of pool, and it provides usefull methods to chain several runnables and execute them in paralell or make some of them wait for otherso to finish and then execute... doesent matter it should be an utility class so that whenever you want you use async() or chain() and dont wory how it works.

Now the sample code : 

public static void main(String[] args) {
    Runtime.getRuntime().addShutdownHook(new Thread() {
        @Override
        public void run() {
            System.out.println("hoooooooooooook");
        }
    });


    ConcurrencyUtil.chain()
            .add(()->System.out.println("task 1"))
            .add(()->System.out.println("task 2"))
            .execute()
            .join();

    System.out.println("main finish");
}

When this executes i'll have this : 

created new thread with name [common-pool : thread-1]
created new thread with name [common-pool : thread-2]
task 1 
task 2 
main finish

As you can see thread pool creates two threads and executes concurrent tasks but those worker threads are still alive (in sleep) waiting for new task, thats why after main execution is finished application is still running... And shutdown hook is not working too as (i suppose) application is considered as alive...

Now given that I have no guarantee that main method is my exit point (it might be a runnable that finishes execution for example if i remove .join() ) thats why i cant put finally block and shut down ConcurrencyUtil...

Any ideas how can this be done properly?

vach
  • 10,571
  • 12
  • 68
  • 106
  • Use a shutdown hook: http://stackoverflow.com/questions/2921945/useful-example-of-a-shutdown-hook-in-java – Boris Pavlović Sep 16 '14 at 12:27
  • 1
    Shutdown hooks should only be used as hacky workarounds for broken code. Properly designed applications have properly designed shutdown procedures. – Marko Topolnik Sep 16 '14 at 12:33
  • I don't know that class. Does it have a constructor that lets you pass in your own ExecutorService? Any library that uses an ExecutorService should provide a means for the client to provide one. (Not saying that they all _do_, but the world would be a better place if they all _did_.) – Solomon Slow Sep 16 '14 at 12:44
  • @MarkoTopolnik I disagree. An embedded database *must* use a shutdown hook. However, I agree that the most people including the OP should not. – maaartinus Sep 16 '14 at 12:47
  • @jameslarge no it has not any constructor the point is that i want to provide its functionality application wide, thus i do it using static methods... only instance here is the class member executor which initializes right at static scope... – vach Sep 16 '14 at 12:49
  • @Vach Static utility classes are usually a code smell. You can always let the user create a `public static` instance if an unlimited access is needed. And maybe they want to mock you class or do something else for what static methods are unusable. – maaartinus Sep 16 '14 at 12:54
  • @maaartinus Why would an embedded database have to use a shutdown hook? The true cause is lack of proper API --> hacks needed as a workaround. – Marko Topolnik Sep 16 '14 at 12:56
  • @maaartinus I'd agree with you, in most cases they are bad design unless they enapsulate and abstract away all the functionality they are providing... And this is exactly this case. But anyway i own this code i can change anything here... All i want is that i have functionality described in update, like without any instance creation id be able to run concurent tasks (manage them) and i dont see any other option to provide this except static methods... no need for mocking as well as it is fully capable for tests... again this is specific case but its a good one... – vach Sep 16 '14 at 13:01
  • @MarkoTopolnik Why not? Would you prefer to get corrupted data instead? What if there's a proper API but the user fails to use it? What if the uses calls shutdown upon normal termination, but the process recieves a TERM signal? Should the user do the add their own hook? – maaartinus Sep 16 '14 at 13:02
  • I guess as @MarkoTopolnik says i need to implement some specific shutdown procedure in my application and do clean there... – vach Sep 16 '14 at 13:02
  • Solved it jentlemen, I've ensured that all treads created in ThreadPoolFactory are deamon threads, so that they dont prevent jvm from shut down... – vach Sep 16 '14 at 13:10
  • Now i'll have nice tool and wont bother creating instances, or shutting it down... – vach Sep 16 '14 at 13:11
  • @maaartinus Of course a shutdown hook is useful *in addition* to a standard mechanism---again as a last-resort measure in case of a failure to follow proper procedure. Most finalizer methods serve the same purpose. – Marko Topolnik Sep 16 '14 at 13:14
  • 1
    @Vach Using daemon threads is dangerous if your tasks have any critical cleanup to perform. Those threads will be abruptly killed at shutdown. Proper design calls for *interruptible* threads. – Marko Topolnik Sep 16 '14 at 13:22
  • @MarkoTopolnik i totally agree, i'll add that into documentation, anyway in my case i think its expected... as it is intended to be used in methods where tasks can be executed concurently. And ususally .join() follows... But I understand your point, didnt know about deamon threads before... – vach Sep 16 '14 at 13:50
  • Maybe it makes sence to change api so that non deamon threads can be created too... so that in that case those non deamons will not be in thread pool but will be created on demand... makes sence? – vach Sep 16 '14 at 13:51
  • @MarkoTopolnik usually when anywhere you use ThreadPool you have some place to call shutdown()? or there is a way to make it garbage collected automatically? – vach Sep 16 '14 at 13:54
  • 1
    I always have a place where I call `shutdown()`. Of course, `ThreadPool` does define a finalizer method, where it calls `shutdown()`. – Marko Topolnik Sep 16 '14 at 13:58
  • I think thats the right way, you can post your answer so that i could mark it so... – vach Sep 16 '14 at 13:59

1 Answers1

1

The proper approach to application design is to introduce an explicit shutdown procedure, which is called from wherever appropriate. This procedure can then call shutdown() on your executor service, followed by awaitTermination(). Look into the Javadoc of ExecutorService for a full code example.

In addition to the regular shutdown protocol you can provide a JVM shutdown hook, which will again initiate the same shutdown procedure. You should never rely on that mechanism, but it is a good thing to have in case of unpredictable failures.

Marko Topolnik
  • 195,646
  • 29
  • 319
  • 436
  • in my case shutdown hook didnt work. It never got executed, not sure that hook is relating to this issue. – vach Sep 16 '14 at 14:09