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

classic Classic list List threaded Threaded
30 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Evan Whelan-2
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

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

Commit messages:
 - 8252883: AccessDeniedException caused by delayed file deletion on Windows

Changes: https://git.openjdk.java.net/jdk/pull/2572/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252883
  Stats: 131 lines in 2 files changed: 124 ins; 5 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2572.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2572/head:pull/2572

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs-2
On Mon, 15 Feb 2021 10:39:32 GMT, Evan Whelan <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Evan Whelan-2
In reply to this post by Evan Whelan-2
> 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

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

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2572/files
  - new: https://git.openjdk.java.net/jdk/pull/2572/files/045d725f..e755d323

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=00-01

  Stats: 36 lines in 2 files changed: 13 ins; 5 del; 18 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2572.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2572/head:pull/2572

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Evan Whelan-2
On Tue, 16 Feb 2021 11:33:08 GMT, Evan Whelan <[hidden email]> 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
>
> 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

Daniel's suggestions have been integrated to the patch

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

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Evan Whelan-2
In reply to this post by Daniel Fuchs-2
On Mon, 15 Feb 2021 12:55:46 GMT, Daniel Fuchs <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs-2
In reply to this post by Evan Whelan-2
On Tue, 16 Feb 2021 11:37:05 GMT, Evan Whelan <[hidden email]> 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
>
> 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

Thanks for taking on the changes. Still some comments...

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

> 73:             handler.close();
> 74:         } catch (Exception e) {
> 75:             throw new RuntimeException(e);

Will throwing an exception in a thread (if happening in the Thread created at line 60) cause jtreg to fail? Or will the exception simply be discarded?

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

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

Same holds there as well. Maybe you could make an experiment with a dummy test to see whether the exception will make the test fail.

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

> 94:                 System.out.println(name + "\t|" + line);
> 95:             }
> 96:             childProcess.waitFor();

Should you be checking the status returned by `waitFor` and fail the test if it's not `0`?

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

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Evan Whelan-2
On Tue, 16 Feb 2021 11:33:08 GMT, Daniel Fuchs <[hidden email]> 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
>
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 75:
>
>> 73:             handler.close();
>> 74:         } catch (Exception e) {
>> 75:             throw new RuntimeException(e);
>
> Will throwing an exception in a thread (if happening in the Thread created at line 60) cause jtreg to fail? Or will the exception simply be discarded?

The exception will in fact make the jtreg test fail. I verified this by creating a dummy test case and throwing an exception within a thread as you suggested.

> test/jdk/java/util/logging/FileHandlerAccessTest.java line 98:
>
>> 96:             childProcess.waitFor();
>> 97:         } catch (Exception e) {
>> 98:             throw new RuntimeException(e);
>
> Same holds there as well. Maybe you could make an experiment with a dummy test to see whether the exception will make the test fail.

Please see above comment. Verified that an exception thrown within a thread causes test to fail

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

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Evan Whelan-2
In reply to this post by Daniel Fuchs-2
On Tue, 16 Feb 2021 11:48:05 GMT, Daniel Fuchs <[hidden email]> 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
>
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 96:
>
>> 94:                 System.out.println(name + "\t|" + line);
>> 95:             }
>> 96:             childProcess.waitFor();
>
> Should you be checking the status returned by `waitFor` and fail the test if it's not `0`?

I'll modify the code to add this behaviour. Thanks for the suggestion :)

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

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Evan Whelan-2
In reply to this post by Evan Whelan-2
> 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

Evan Whelan has updated the pull request incrementally with one additional commit since the last revision:

  8252883: Throw exception if exit code of child process is non-zero

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2572/files
  - new: https://git.openjdk.java.net/jdk/pull/2572/files/e755d323..9472f73c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=01-02

  Stats: 9 lines in 1 file changed: 4 ins; 1 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2572.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2572/head:pull/2572

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

David Holmes-2
On Tue, 16 Feb 2021 21:15:58 GMT, Evan Whelan <[hidden email]> 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
>
> Evan Whelan has updated the pull request incrementally with one additional commit since the last revision:
>
>   8252883: Throw exception if exit code of child process is non-zero

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

> 7:  * published by the Free Software Foundation.  Oracle designates this
> 8:  * particular file as subject to the "Classpath" exception as provided
> 9:  * by Oracle in the LICENSE file that accompanied this code.

Tests do not need, and should not have, the CPE.

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

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Evan Whelan-2
In reply to this post by Evan Whelan-2
> 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

Evan Whelan has updated the pull request incrementally with one additional commit since the last revision:

  8252883: Remove ClassPathException copyright statement

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2572/files
  - new: https://git.openjdk.java.net/jdk/pull/2572/files/9472f73c..26c56c47

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=02-03

  Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2572.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2572/head:pull/2572

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Evan Whelan-2
In reply to this post by David Holmes-2
On Wed, 17 Feb 2021 07:37:42 GMT, David Holmes <[hidden email]> wrote:

>> Evan Whelan has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8252883: Throw exception if exit code of child process is non-zero
>
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 9:
>
>> 7:  * published by the Free Software Foundation.  Oracle designates this
>> 8:  * particular file as subject to the "Classpath" exception as provided
>> 9:  * by Oracle in the LICENSE file that accompanied this code.
>
> Tests do not need, and should not have, the CPE.

Fixed, thanks

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

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs-2
In reply to this post by Evan Whelan-2
On Wed, 17 Feb 2021 11:50:05 GMT, Evan Whelan <[hidden email]> 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
>
> Evan Whelan has updated the pull request incrementally with one additional commit since the last revision:
>
>   8252883: Remove ClassPathException copyright statement

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

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

Ah - sorry - since this is a test, instead of return you should probably throw an exception - e.g.:
throw new IllegalArgumentException("Usage error: expects java FileHandlerAccessTest [process/thread] <count>");

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

> 45:             return;
> 46:         }
> 47:         else if (args.length == 2) {

nit: `} else if (...) {`

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

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Evan Whelan-2
In reply to this post by Evan Whelan-2
> 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

Evan Whelan has updated the pull request incrementally with one additional commit since the last revision:

  8252883: Added IllegalArgumentException for incorrect usage

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2572/files
  - new: https://git.openjdk.java.net/jdk/pull/2572/files/26c56c47..b5259dcb

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=03-04

  Stats: 6 lines in 1 file changed: 0 ins; 3 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2572.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2572/head:pull/2572

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Evan Whelan-2
In reply to this post by Daniel Fuchs-2
On Wed, 17 Feb 2021 12:24:46 GMT, Daniel Fuchs <[hidden email]> wrote:

>> Evan Whelan has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8252883: Remove ClassPathException copyright statement
>
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 45:
>
>> 43:         if (!(args.length == 2 || args.length == 1)) {
>> 44:             System.out.println("Usage error: expects java FileHandlerAccessTest [process/thread] <count>");
>> 45:             return;
>
> Ah - sorry - since this is a test, instead of return you should probably throw an exception - e.g.:
> throw new IllegalArgumentException("Usage error: expects java FileHandlerAccessTest [process/thread] <count>");

Done! Thanks Daniel

> test/jdk/java/util/logging/FileHandlerAccessTest.java line 47:
>
>> 45:             return;
>> 46:         }
>> 47:         else if (args.length == 2) {
>
> nit: `} else if (...) {`

Fixed

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

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs-2
In reply to this post by Evan Whelan-2
On Wed, 17 Feb 2021 14:47:03 GMT, Evan Whelan <[hidden email]> 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
>
> Evan Whelan has updated the pull request incrementally with one additional commit since the last revision:
>
>   8252883: Added IllegalArgumentException for incorrect usage

LGTM

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

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs-2
On Thu, 18 Feb 2021 10:41:17 GMT, Daniel Fuchs <[hidden email]> wrote:

>> Evan Whelan has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8252883: Added IllegalArgumentException for incorrect usage
>
> LGTM

I'm seeing some intermittent test failures with the new test (after merging in latest master changes). Can you retest and have a look:

----------System.out:(25/1343)----------
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Testing with arguments: type=process, count=20
Thread-2 |Error: Could not find or load main class FileHandlerAccessTest
Thread-2 |Caused by: java.lang.ClassNotFoundException: FileHandlerAccessTest
Thread-6 |Error: Could not find or load main class FileHandlerAccessTest
Thread-6 |Caused by: java.lang.ClassNotFoundException: FileHandlerAccessTest
Thread-1 |Error: Could not find or load main class FileHandlerAccessTest
Thread-1 |Caused by: java.lang.ClassNotFoundException: FileHandlerAccessTest
Testing with arguments: type=process, count=20
----------System.err:(14/1024)----------
java.lang.RuntimeException: java.lang.RuntimeException: An error occured in the child process.
        at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:96)
        at java.base/java.lang.Thread.run(Thread.java:831)
Caused by: java.lang.RuntimeException: An error occured in the child process.
        at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:93)
        ... 1 more
STATUS:Failed.`main' threw exception: java.lang.RuntimeException: java.lang.RuntimeException: An error occured in the child process.
java.lang.RuntimeException: java.lang.RuntimeException: An error occured in the child process.
        at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:96)
        at java.base/java.lang.Thread.run(Thread.java:831)
Caused by: java.lang.RuntimeException: An error occured in the child process.
        at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:93)
        ... 1 more
STATUS:Failed.`main' threw exception: java.lang.RuntimeException: java.lang.RuntimeException: An error occured in the child process.
----------rerun:(39/6191)*----------

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

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Evan Whelan-2
In reply to this post by Evan Whelan-2
> 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

Evan Whelan has updated the pull request incrementally with one additional commit since the last revision:

  8252883: Stripped back FileHandlerAccessTest to only use threads

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2572/files
  - new: https://git.openjdk.java.net/jdk/pull/2572/files/b5259dcb..1bb8837e

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=04-05

  Stats: 58 lines in 1 file changed: 0 ins; 53 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2572.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2572/head:pull/2572

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Evan Whelan-2
In reply to this post by Daniel Fuchs-2
On Fri, 19 Feb 2021 14:06:23 GMT, Daniel Fuchs <[hidden email]> wrote:

>> LGTM
>
> I'm seeing some intermittent test failures with the new test (after merging in latest master changes). Can you retest and have a look:
>
> ----------System.out:(25/1343)----------
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Testing with arguments: type=process, count=20
> Thread-2 |Error: Could not find or load main class FileHandlerAccessTest
> Thread-2 |Caused by: java.lang.ClassNotFoundException: FileHandlerAccessTest
> Thread-6 |Error: Could not find or load main class FileHandlerAccessTest
> Thread-6 |Caused by: java.lang.ClassNotFoundException: FileHandlerAccessTest
> Thread-1 |Error: Could not find or load main class FileHandlerAccessTest
> Thread-1 |Caused by: java.lang.ClassNotFoundException: FileHandlerAccessTest
> Testing with arguments: type=process, count=20
> ----------System.err:(14/1024)----------
> java.lang.RuntimeException: java.lang.RuntimeException: An error occured in the child process.
> at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:96)
> at java.base/java.lang.Thread.run(Thread.java:831)
> Caused by: java.lang.RuntimeException: An error occured in the child process.
> at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:93)
> ... 1 more
> STATUS:Failed.`main' threw exception: java.lang.RuntimeException: java.lang.RuntimeException: An error occured in the child process.
> java.lang.RuntimeException: java.lang.RuntimeException: An error occured in the child process.
> at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:96)
> at java.base/java.lang.Thread.run(Thread.java:831)
> Caused by: java.lang.RuntimeException: An error occured in the child process.
> at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:93)
> ... 1 more
> STATUS:Failed.`main' threw exception: java.lang.RuntimeException: java.lang.RuntimeException: An error occured in the child process.
> ----------rerun:(39/6191)*----------

Hi,

I've removed the problematic "process" handling logic and have stripped the test back to only use threads.

The problem was reproducible (intermittently on my Windows machine) using this smaller test and should make the test more stable.

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

PR: https://git.openjdk.java.net/jdk/pull/2572
Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs-2
On Mon, 22 Feb 2021 09:46:56 GMT, Evan Whelan <[hidden email]> wrote:

>> I'm seeing some intermittent test failures with the new test (after merging in latest master changes). Can you retest and have a look:
>>
>> ----------System.out:(25/1343)----------
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Testing with arguments: type=process, count=20
>> Thread-2 |Error: Could not find or load main class FileHandlerAccessTest
>> Thread-2 |Caused by: java.lang.ClassNotFoundException: FileHandlerAccessTest
>> Thread-6 |Error: Could not find or load main class FileHandlerAccessTest
>> Thread-6 |Caused by: java.lang.ClassNotFoundException: FileHandlerAccessTest
>> Thread-1 |Error: Could not find or load main class FileHandlerAccessTest
>> Thread-1 |Caused by: java.lang.ClassNotFoundException: FileHandlerAccessTest
>> Testing with arguments: type=process, count=20
>> ----------System.err:(14/1024)----------
>> java.lang.RuntimeException: java.lang.RuntimeException: An error occured in the child process.
>> at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:96)
>> at java.base/java.lang.Thread.run(Thread.java:831)
>> Caused by: java.lang.RuntimeException: An error occured in the child process.
>> at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:93)
>> ... 1 more
>> STATUS:Failed.`main' threw exception: java.lang.RuntimeException: java.lang.RuntimeException: An error occured in the child process.
>> java.lang.RuntimeException: java.lang.RuntimeException: An error occured in the child process.
>> at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:96)
>> at java.base/java.lang.Thread.run(Thread.java:831)
>> Caused by: java.lang.RuntimeException: An error occured in the child process.
>> at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:93)
>> ... 1 more
>> STATUS:Failed.`main' threw exception: java.lang.RuntimeException: java.lang.RuntimeException: An error occured in the child process.
>> ----------rerun:(39/6191)*----------
>
> Hi,
>
> I've removed the problematic "process" handling logic and have stripped the test back to only use threads.
>
> The problem was reproducible (intermittently on my Windows machine) using this smaller test and should make the test more stable.

Does the new version of the test still occasionally catches the issue and fails if the fix is not present?

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

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