RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v2]

[Evan Whelan]

On Mon, 15 Feb 2021 12:55:46 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> Evan Whelan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8252883: Doc cleanup, code formatting and throwing exceptions instead of catching
>
> 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?

Yes, reverted thanks

> 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
>                         } ...

I agree. I've updated the comments

> 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.

Changed to `return`

> 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?

Thanks for catching this. 

I've updated the test to throw the exceptions rather than printing stack trace. This was a modified version of the test for debugging purposes.

It's worth noting that it's not possible to write a test which can deterministically prove/ verify the behaviour (and related fix) of the parent bug as this involves the situation where multiple threads conflict on a Windows machine.

The test attached is a best-effort and was tried around 40 times.

> test/jdk/java/util/logging/FileHandlerAccessTest.java line 98:
> 
>> 96:             childProcess.waitFor();
>> 97:         }
>> 98:         catch(Exception e) {
> 
> space missing after `catch`

Fixed

> 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?

Thanks Daniel, please see my above reply

> 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 {
>         ...
>     }

Fixed

> test/jdk/java/util/logging/FileHandlerAccessTest.java line 106:
> 
>> 104:             }
>> 105:             if (bufferedReader != null){
>> 106:                 try{
> 
> space missing after `try`

Fixed

> test/jdk/java/util/logging/FileHandlerAccessTest.java line 109:
> 
>> 107:                     bufferedReader.close();
>> 108:                 }
>> 109:                 catch(Exception ignored){}
> 
> space missing after `catch`

Fixed

> test/jdk/java/util/logging/FileHandlerAccessTest.java line 113:
> 
>> 111:         }
>> 112:     }
>> 113: }
> 
> Please add a newline at the end of the file.

Done

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

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