On Mon, 15 Feb 2021 12:55:46 GMT, Daniel Fuchs <dfu...@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