RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows

[Daniel Fuchs]

On Mon, 15 Feb 2021 10:39:32 GMT, Evan Whelan <ewhelan at openjdk.org> wrote:

> Hi,
> 
> Please review this fix for JDK-8252883. This handles the case when an AccessDeniedException is being thrown on Windows, due to a delay in deleting the lock file it is trying to write to.
> 
> This fix passes all testing.
> 
> Kind regards,
> Evan

src/java.logging/share/classes/java/util/logging/FileHandler.java line 39:

> 37: import java.nio.channels.FileChannel;
> 38: import java.nio.channels.OverlappingFileLockException;
> 39: import java.nio.file.*;

We avoid using the wildcard with imports in the JDK sources. Could you revert that change?

src/java.logging/share/classes/java/util/logging/FileHandler.java line 517:

> 515:                             continue;
> 516:                         }
> 517:                         else {

nit: can you put the closing brace and the else on the same line?

src/java.logging/share/classes/java/util/logging/FileHandler.java line 512:

> 510:                         // Try again. If it doesn't work, then this will
> 511:                         // eventually ensure that we increment "unique" and
> 512:                         // use another file name.

I believe this would be more clear if the comment was put inside the if statement, just before continue.
It might be good to add some words about what might be causing the AccessDeniedException as well... 
Maybe something like:

                    } catch (AccessDeniedException ade) {
                        // This can be either a temporary, or a more permanent issue.
                        // The lock file might be still pending deletion from a previous run
                        // (temporary), or the parent directory might not be accessible, etc...
                        // If we can write to the current directory, and this is a regular file, 
                        // let's try again.
                        if (Files.isRegularFile(lockFilePath, LinkOption.NOFOLLOW_LINKS)
                            && isParentWritable(lockFilePath)) {
                            // Try again. If it doesn't work, then this will
                            // eventually ensure that we increment "unique" and
                            // use another file name.
                            continue;
                        } else {
                            throw ade; // no need to retry
                        } ...

test/jdk/java/util/logging/FileHandlerAccessTest.java line 47:

> 45:         if (!(args.length == 2 || args.length == 1)) {
> 46:             System.out.println("Usage error: expects java FileHandlerAccessTest [process/thread] <count>");
> 47:             System.exit(1);

We usually avoid `System.exit()` in tests. `return` should be enough.

test/jdk/java/util/logging/FileHandlerAccessTest.java line 113:

> 111:         }
> 112:     }
> 113: }

Please add a newline at the end of the file.

test/jdk/java/util/logging/FileHandlerAccessTest.java line 93:

> 91:             bufferedReader = new BufferedReader(new InputStreamReader(childProcess.getInputStream()));
> 92:             String line;
> 93:             while((line = bufferedReader.readLine()) != null) {

space missing after `while`

test/jdk/java/util/logging/FileHandlerAccessTest.java line 98:

> 96:             childProcess.waitFor();
> 97:         }
> 98:         catch(Exception e) {

space missing after `catch`

test/jdk/java/util/logging/FileHandlerAccessTest.java line 106:

> 104:             }
> 105:             if (bufferedReader != null){
> 106:                 try{

space missing after `try`

test/jdk/java/util/logging/FileHandlerAccessTest.java line 109:

> 107:                     bufferedReader.close();
> 108:                 }
> 109:                 catch(Exception ignored){}

space missing after `catch`

test/jdk/java/util/logging/FileHandlerAccessTest.java line 101:

> 99:             e.printStackTrace();
> 100:         }
> 101:         finally {

I would prefer if `catch` and `finally` where on the same line than the preceding closing brace:

    try {
        ...
    } catch (...) {
        ...
    } finally {
        ...
    }

test/jdk/java/util/logging/FileHandlerAccessTest.java line 75:

> 73:         }
> 74:         catch(Exception e) {
> 75:             e.printStackTrace();

If you only print exceptions, when does the test fail?

test/jdk/java/util/logging/FileHandlerAccessTest.java line 99:

> 97:         }
> 98:         catch(Exception e) {
> 99:             e.printStackTrace();

Same remark here: should this make the test fail?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2572