On Mon, 15 Feb 2021 12:55:46 GMT, Daniel Fuchs <[email protected]> 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
