patternjavaMinor
Delete temp files on startup
Viewed 0 times
tempfilesdeletestartup
Problem
I wrote some code for my question about deleting temp files that weren't deleted on exit before and would like to get some feedback if I forgot some details or got something wrong.
```
private static final String PROJECTNAME = "projectname";
private static final String LOCKNAME = "lock_ThisIsAVeryLongFilenameNoOtherApplicationWillUse.lock";
private static void initTempDir(Path tempPath) throws IOException {
final Path tempFolder = Files.createTempDirectory(tempPath, PROJECTNAME);
Runtime.getRuntime().addShutdownHook(new Thread(() -> {
try {
DeepDelete.deepDelete(tempFolder);
} catch (IOException e) {
e.printStackTrace();
}
}));
Timer timer = new Timer(true);
timer.scheduleAtFixedRate(new TimerTask() {
@Override
public void run() {
Path lockFile = tempFolder.resolve(LOCKNAME);
try {
Files.write(lockFile, ByteBuffer.allocate(Long.BYTES).putLong(System.currentTimeMillis()).array());
} catch (IOException e) {
System.err.println("Failed to secure avaliability of temp folder, exiting!");
e.printStackTrace();
System.exit(-1);
}
}
}, 60_000, 60_000);
// Make first write manually to be sure the file has been written when
// old temp files get deleted. Otherwise, the current temp file not
// having a time stamp (yet) would be classified as old and removed.
Files.write(tempFolder.resolve(LOCKNAME),
ByteBuffer.allocate(Long.BYTES).putLong(System.currentTimeMillis()).array());
long currentTime = System.currentTimeMillis();
Files.list(tempFolder.getParent())//
.filter(Files::isDirectory)//
.filter(p -> p.getFileName().toString().startsWith(PROJECTNAME))//
.filter(p -> {
Path lock = p.resolve(LOCKNAME);
if (!Files.isRegularFile(lock))
return true;
```
private static final String PROJECTNAME = "projectname";
private static final String LOCKNAME = "lock_ThisIsAVeryLongFilenameNoOtherApplicationWillUse.lock";
private static void initTempDir(Path tempPath) throws IOException {
final Path tempFolder = Files.createTempDirectory(tempPath, PROJECTNAME);
Runtime.getRuntime().addShutdownHook(new Thread(() -> {
try {
DeepDelete.deepDelete(tempFolder);
} catch (IOException e) {
e.printStackTrace();
}
}));
Timer timer = new Timer(true);
timer.scheduleAtFixedRate(new TimerTask() {
@Override
public void run() {
Path lockFile = tempFolder.resolve(LOCKNAME);
try {
Files.write(lockFile, ByteBuffer.allocate(Long.BYTES).putLong(System.currentTimeMillis()).array());
} catch (IOException e) {
System.err.println("Failed to secure avaliability of temp folder, exiting!");
e.printStackTrace();
System.exit(-1);
}
}
}, 60_000, 60_000);
// Make first write manually to be sure the file has been written when
// old temp files get deleted. Otherwise, the current temp file not
// having a time stamp (yet) would be classified as old and removed.
Files.write(tempFolder.resolve(LOCKNAME),
ByteBuffer.allocate(Long.BYTES).putLong(System.currentTimeMillis()).array());
long currentTime = System.currentTimeMillis();
Files.list(tempFolder.getParent())//
.filter(Files::isDirectory)//
.filter(p -> p.getFileName().toString().startsWith(PROJECTNAME))//
.filter(p -> {
Path lock = p.resolve(LOCKNAME);
if (!Files.isRegularFile(lock))
return true;
Solution
The code looks good and should work properly. Here is some feedback nevertheless.
I don't really understand why the file is named as a "lock". A lock file is typically used to prevent multiple instances of a program to run, or synchronise acess to a resource between different processes.
Usage of Timer
The
Deletion of tempFolder
Make sure that the
Duplicated code
Extract the following to a method, which appears twice. It will also do always the error handling:
Hardcoded values
Define hardcoded values in constants. It will be easier to maintain.
Read safely
The entire file will be read to memory and only then the first
It would be better to only read the first
Inconsistency?
Looking at
There seems to be an inconsistency. If the file doesn't exist, the directory is kept. If the file exists but its content is unexpected, we delete the directory. Both situations represent a corrupted temporary folder of the program and it seems to me that both situations should have the same handling.
I don't really understand why the file is named as a "lock". A lock file is typically used to prevent multiple instances of a program to run, or synchronise acess to a resource between different processes.
Usage of Timer
The
Timer was effectivelly replaces by Executors, more specifically ScheduledThreadPoolExecutor as mentioned in the javadoc of Timer. Oneimmediate advantage is that you can submit tasks as Runnable with a lambda expression.Deletion of tempFolder
Make sure that the
DeepDelete.deepDelete(tempFolder); does not fail if tempFolder doesn't exist. If the application terminates due to an error (such as the failure to write to tempFolder, you would have a second exception.Duplicated code
Extract the following to a method, which appears twice. It will also do always the error handling:
private static void writeFile(Path tempFolder) {
try {
Path lockFile = tempFolder.resolve(LOCKNAME);
Files.write(lockFile, ByteBuffer.allocate(Long.BYTES).putLong(System.currentTimeMillis()).array());
} catch (IOException e) {
System.err.println("Failed to secure avaliability of temp folder, exiting!");
e.printStackTrace();
System.exit(-1);
}
}Hardcoded values
Define hardcoded values in constants. It will be easier to maintain.
private static final Integer DELAY_MS = 60_000;
private static final Integer DELETE_THRESHOLD_MS = 5 * 60_000;Read safely
ByteBuffer.wrap(Files.readAllBytes(lock)).getLong() < currentTime - 60_000 * 5;The entire file will be read to memory and only then the first
Long.BYTES bytes are read as a long. This could be a problem if the file happens to be very large, even though it's not expected.It would be better to only read the first
Long.BYTES of the file.Inconsistency?
Looking at
if (!Files.isRegularFile(lock))
return true;
try {
return ByteBuffer.wrap(Files.readAllBytes(lock)).getLong() < currentTime - 60_000 * 5;
} catch (IOException e) {
e.printStackTrace();
return false;
}There seems to be an inconsistency. If the file doesn't exist, the directory is kept. If the file exists but its content is unexpected, we delete the directory. Both situations represent a corrupted temporary folder of the program and it seems to me that both situations should have the same handling.
Code Snippets
private static void writeFile(Path tempFolder) {
try {
Path lockFile = tempFolder.resolve(LOCKNAME);
Files.write(lockFile, ByteBuffer.allocate(Long.BYTES).putLong(System.currentTimeMillis()).array());
} catch (IOException e) {
System.err.println("Failed to secure avaliability of temp folder, exiting!");
e.printStackTrace();
System.exit(-1);
}
}private static final Integer DELAY_MS = 60_000;
private static final Integer DELETE_THRESHOLD_MS = 5 * 60_000;ByteBuffer.wrap(Files.readAllBytes(lock)).getLong() < currentTime - 60_000 * 5;if (!Files.isRegularFile(lock))
return true;
try {
return ByteBuffer.wrap(Files.readAllBytes(lock)).getLong() < currentTime - 60_000 * 5;
} catch (IOException e) {
e.printStackTrace();
return false;
}Context
StackExchange Code Review Q#160002, answer score: 2
Revisions (0)
No revisions yet.