On Mon, 15 Feb 2021 10:39:32 GMT, Evan Whelan <ewhe...@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
