2

This code :

File tmpFile = File.createTempFile(PREFIX_TMP, null, new File(reportPath));
        logger.debug("The report will be saved in the temporary file '{}'", tmpFile.getName());

        reportWriter.write(tmpFile, report);

        Calendar calendar = Calendar.getInstance();

        boolean isFileMoved = false;
        while (!isFileMoved) {
            String reportName = String.format(report.getName(), calendar.getTime());
            File reportFile = new File(reportPath, reportName);

            try {
                Files.move(tmpFile.toPath(), reportFile.toPath());
                isFileMoved = true;

                logger.info("The temporary file '{}' is renamed to '{}'", tmpFile.getName(), reportFile.getName());
            } catch (FileAlreadyExistsException e) {
                logger.warn("The file '{}' already exists in the folder '{}': adding a second to generation time",
                        reportName, reportPath);
            }

            calendar.add(Calendar.SECOND, 1);
        }

generates the following, impossible, log statements :

2016-10-04 10:35:11,674 [WARN ] [_Executor-1] a.b.c.MyGenerator - The file 'myFile_01092016103511.csv' already exists in the folder '/myDir': adding a second to generation time

2016-10-04 10:35:11,677 [WARN ] [_Executor-3] a.b.c.MyGenerator - The file 'myFile_01092016103511.csv' already exists in the folder '/myDir': adding a second to generation time

2016-10-04 10:35:11,677 [INFO ] [_Executor-1] a.b.c.MyGenerator - The temporary file 'tmp2103892505851032137.tmp' is renamed to 'myFile_01092016103512.csv'

2016-10-04 10:35:11,680 [INFO ] [_Executor-3] a.b.c.MyGenerator - The temporary file 'tmp6843688962754506611.tmp' is renamed to 'myFile_01092016103512.csv'

Executor 3 overwrities the file, even though it should throw a FileAlreadyExistsException.

No exception is thrown, and the file's data has been overwritten.

Aditya W
  • 652
  • 8
  • 20
NimChimpsky
  • 46,453
  • 60
  • 198
  • 311

2 Answers2

1

Executor 3 overwrites the file, even though it should throw a FileAlreadyExistsException.

You need to use the ATOMIC_MOVE option if you want the Files.move(...) to act atomically. By default, it first tests if the destination exists and then does the move which is susceptible to thread race conditions. Here are the javadocs for Files.move(...):

ATOMIC_MOVE – Performs the move as an atomic file operation. If the file system does not support an atomic move, an exception is thrown. With an ATOMIC_MOVE you can move a file into a directory and be guaranteed that any process watching the directory accesses a complete file.

If you must have the FileAlreadyExistsException support then you will need to protect the moves with a synchronized block to only allow one thread to be running it at a time. Given you are doing IO in this block, the synchronized should be little to no performance penalty.

Community
  • 1
  • 1
Gray
  • 115,027
  • 24
  • 293
  • 354
0

Files.move uses Files.exists internally, the status could have changed in between when using multiple threads.

ravthiru
  • 8,878
  • 2
  • 43
  • 52