4

I have a simple java program that creates a series of temporary files stored in a local tmp directory. I have added a simple shutdown hook that walks through all files and deletes them, then deletes the tmp directory, before exiting the program. here is the code:

Runtime.getRuntime().addShutdownHook(new Thread(new Runnable() {
    @Override
    public void run() {
        File tmpDir = new File("tmp/");
        for (File f : tmpDir.listFiles()) {
            f.delete();
        }
        tmpDir.delete();
    }
}));

My problem is that the thread that creates these files may not have terminated upon launch of the shutdown hook, and therefore, there may be a file created after listFiles() is called. this causes the tmp dir not to get deleted. I have come up with 2 hacks around this:

Hack # 1:

Runtime.getRuntime().addShutdownHook(new Thread(new Runnable() {
    @Override
    public void run() {
        File tmpDir = new File("tmp/");
        while (!tmp.delete()){
                for (File f : tmpDir.listFiles()) {
                f.delete();
            }
        }
    }
}));

Hack # 2:

Runtime.getRuntime().addShutdownHook(new Thread(new Runnable() {
    @Override
    public void run() {
        try{
            Thread.sleep(1000);
        } catch(InterruptedException e){
            e.printStackTrace();
        }
        File tmpDir = new File("tmp/");
        for (File f : tmpDir.listFiles()) {
            f.delete();
        }
            tmpDir.delete();
        }
}));

Neither is a particularly good solution. What would be ideal is to have the shutdown hook wait until all threads have terminated before continuing. Does anyone know if this can be done?

ewok
  • 20,148
  • 51
  • 149
  • 254
  • 4
    What about having each thread clean up its own temp files? I've found it helpful to have the same class responsible for creating and deleting its own temp files. – ben_w Jul 19 '12 at 19:04
  • What results do you get from [`File.deleteOnExit()`](http://docs.oracle.com/javase/6/docs/api/java/io/File.html#deleteOnExit())? It seems like the "normal termination" requirement would apply equally to this as to shutdown hook execution. – erickson Jul 19 '12 at 19:05
  • @erickson `File.deleteOnExit();` works for the subfiles, but does not work for the tmp directory. presumably the reason is there is no guarantee that all subfiles will be deleted prior to the directory attempting to be deleted. this leads to a nonempty directory, which java cannot delete. – ewok Jul 19 '12 at 19:46
  • @ben_w that is typically a good idea, but the structure of my application has a background thread create files that another thread will use. therefore, if they get deleted upon termination of the background thread, the app will break – ewok Jul 19 '12 at 19:54
  • Sharing temp files across threads and using a **ShutdownHook** to paper over the gaps in resource management is a very bad idea. Reconsider the original design. – Alain O'Dea Jun 09 '14 at 18:31

3 Answers3

7

Just keep track of all your running threads and then.join() them before shutting down the program.

This is an answer to the question title as the ewok has said he can't use .deleteOnExit()

Mitch Connor
  • 766
  • 10
  • 19
  • That is a very bad idea. It is almost guaranteed to make interrupt non-terminating. – Alain O'Dea Jun 05 '14 at 14:00
  • I'm sorry I don't understand what you mean by this. Can you please elaborate or offer your own answer about how to do this correctly? – Mitch Connor Jun 05 '14 at 16:08
  • Short answer: threads are a terrible concurrency abstraction in the presence of global mutable state and should die in a fire. If any one of the threads are in code that doesn't check for interrupts or handle them correctly (a very large proportion of Java code I've seen in production), then calling **.join()** will block forever. Better to join with timeouts and fail to delete the temp files. There's a better solution, which I've posted. – Alain O'Dea Jun 07 '14 at 01:37
  • Your answer is fair within the bounds of the question so I've retracted my downvote. I still think ewok's original solution sharing temp files across threads and using a **ShutdownHook** is doomed to failure and needs a total rewrite. – Alain O'Dea Jun 09 '14 at 18:17
  • I can only help people as much as they will allow me. – Mitch Connor Jun 09 '14 at 18:24
2

What Tyler said, but with a little more detail:

  • Keep references to the threads where the shutdown hook can access them.
  • Have the shutdown hook call interrupt on the threads.
  • Review the code of the threads to make sure they actually respond to interruption (instead of eating the InterruptedException and blundering on, which is typical of a lot of code). An interrupt should prompt the thread to stop looping or blocking, wrap up unfinished business, and terminate.
  • For each thread where you don't want to proceed until it finishes, check whether the thread is alive and if so call join on it, setting a timeout in case it doesn't finish in a reasonable time, in which case you can decide whether to delete the file or not.
Nathan Hughes
  • 94,330
  • 19
  • 181
  • 276
2

UPDATE: Tyler Heiks accurately pointed out that deleteOnExit() isn't a valid solution since the OP tried it and it did not work. I am providing an alternate solution. It is again indirect, but mainly because the original design using threads and a ShutdownHook is fatally flawed.

Use finally blocks to delete the temp files.

Relying on ShutdownHooks for resource management is a very bad idea and makes the code very difficult to compose or reuse in a larger system. It's an even worse idea to hand resources from thread to thread. Resources like files and streams are among the most dangerous things to share between threads. There is likely very little to gain from this and it would make far more sense for each thread to independently obtain temp files using the library's createTempFile methods and manage their use and deletion using try/finally.

The convention for dealing with the temporary files on the system is to treat them as block boxes where:

  1. location on disk is opaque (irrelevant to and not used directly by the program)
  2. filename is irrelevant
  3. filename is guaranteed to be mutually exclusive

The third above is very difficult to achieve if you hand-roll code to create and name temp files yourself. It is likely to be brittle and fail at the worst times (3AM pager anyone?).

The algorithm you present could delete files created by other processes that coincidentally share the same parent directory. That is unlikely to be a good thing for the stability of those other programs.

Here's the high-level process:

  1. Get Path with Files.createTempFile() (or with legacy pre-Java 7 code File with File.createTempFile())
  2. Use temp file however desired
  3. Delete file

This is similar to InputStream or other resources that need to be manually managed.

That general pattern for explicit resource management (when AutoCloseable and try-with-resources aren't available) is as follows.

Resource r = allocateResource();
try {
   useResource(r);
} finally {
   releaseResource(r);
}

In the case of Path it looks like this:

Path tempDir = Paths.get("tmp/);
try {
    Path p = Files.createTempFile(tempDir, "example", ".tmp");
    try {
       useTempFile(f);
    } finally {
       Files.delete(f);
    }
} finally {
    Files.delete(tempDir);
}

On pre-Java 7 legacy, the use with File looks like this:

File tempDir = new File("tmp/");
try {
    File f = File.createTempFile(tempDir, "example", ".tmp");
    try {
       useTempFile(f);
    } finally {
       if (!f.delete()) {
          handleFailureToDeleteTempFile(f);
       }
    }
} finally {
    if (!tempDir.delete()) {
        handleFailureToDeleteTempDir(tempDir);
    }
}
Community
  • 1
  • 1
Alain O'Dea
  • 21,033
  • 1
  • 58
  • 84
  • OP stated that deleteOnExit() was not an option and didn't work therefore we provided an alternative way. Also, I was answering the question in the title. – Mitch Connor Jun 07 '14 at 09:40
  • Touché. I didn't read the comments. This is a great example of the consequences of hand-managing subdirectories of temp. It may be better to just use individual temp files and simulate hierarchies as needed using in-memory data structures. – Alain O'Dea Jun 08 '14 at 02:24
  • Would you mind reversing your down-vote on my original answer? – Mitch Connor Jun 08 '14 at 05:23