RFR 8184961: jdk.test.lib.util.FileUtils.deleteFileWithRetry0 should wait for absence of a file

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR 8184961: jdk.test.lib.util.FileUtils.deleteFileWithRetry0 should wait for absence of a file

Andrey Nazarov
Hi,

Could you review fix in test library which should help to resolve intermittent issues on Windows machines during directories clean up.
 
Bug: https://bugs.openjdk.java.net/browse/JDK-8184961 <https://bugs.openjdk.java.net/browse/JDK-8184961>
Review: http://cr.openjdk.java.net/~anazarov/JDK-8184961/webrev.00/webrev/


—Andrei
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8184961: jdk.test.lib.util.FileUtils.deleteFileWithRetry0 should wait for absence of a file

Andrey Nazarov
Can anyone look?

—Thanks,
Andrei

> On 19 Jul 2017, at 18:21, Andrey Nazarov <[hidden email]> wrote:
>
> Hi,
>
> Could you review fix in test library which should help to resolve intermittent issues on Windows machines during directories clean up.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8184961 <https://bugs.openjdk.java.net/browse/JDK-8184961>
> Review: http://cr.openjdk.java.net/~anazarov/JDK-8184961/webrev.00/webrev/
>
>
> —Andrei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8184961: jdk.test.lib.util.FileUtils.deleteFileWithRetry0 should wait for absence of a file

Brian Burkhalter-2
Hi Andrei,

I think this looks good.

Thanks,

Brian

On Jul 25, 2017, at 3:59 PM, Andrey Nazarov <[hidden email]> wrote:

> Can anyone look?
>
> —Thanks,
> Andrei
>> On 19 Jul 2017, at 18:21, Andrey Nazarov <[hidden email]> wrote:
>>
>> Hi,
>>
>> Could you review fix in test library which should help to resolve intermittent issues on Windows machines during directories clean up.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8184961 <https://bugs.openjdk.java.net/browse/JDK-8184961>
>> Review: http://cr.openjdk.java.net/~anazarov/JDK-8184961/webrev.00/webrev/
>>
>>
>> —Andrei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8184961: jdk.test.lib.util.FileUtils.deleteFileWithRetry0 should wait for absence of a file

Andrey Nazarov
Thanks, Brian

—Andrei

> On 25 Jul 2017, at 16:41, Brian Burkhalter <[hidden email]> wrote:
>
> Hi Andrei,
>
> I think this looks good.
>
> Thanks,
>
> Brian
>
> On Jul 25, 2017, at 3:59 PM, Andrey Nazarov <[hidden email]> wrote:
>
>> Can anyone look?
>>
>> —Thanks,
>> Andrei
>>> On 19 Jul 2017, at 18:21, Andrey Nazarov <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>> Could you review fix in test library which should help to resolve intermittent issues on Windows machines during directories clean up.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8184961 <https://bugs.openjdk.java.net/browse/JDK-8184961>
>>> Review: http://cr.openjdk.java.net/~anazarov/JDK-8184961/webrev.00/webrev/
>>>
>>>
>>> —Andrei
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8184961: jdk.test.lib.util.FileUtils.deleteFileWithRetry0 should wait for absence of a file

Hohensee, Paul
A double negative (!Files.notExists) somewhat unusual coding and will perplex people reading it. They might even switch it back to Files.exists as a style cleanup, so I recommend adding a comment explaining why you’re using it.

Thanks,

Paul

On 7/25/17, 5:08 PM, "core-libs-dev on behalf of Andrey Nazarov" <[hidden email] on behalf of [hidden email]> wrote:

    Thanks, Brian
   
    —Andrei
    > On 25 Jul 2017, at 16:41, Brian Burkhalter <[hidden email]> wrote:
    >
    > Hi Andrei,
    >
    > I think this looks good.
    >
    > Thanks,
    >
    > Brian
    >
    > On Jul 25, 2017, at 3:59 PM, Andrey Nazarov <[hidden email]> wrote:
    >
    >> Can anyone look?
    >>
    >> —Thanks,
    >> Andrei
    >>> On 19 Jul 2017, at 18:21, Andrey Nazarov <[hidden email]> wrote:
    >>>
    >>> Hi,
    >>>
    >>> Could you review fix in test library which should help to resolve intermittent issues on Windows machines during directories clean up.
    >>>
    >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8184961 <https://bugs.openjdk.java.net/browse/JDK-8184961>
    >>> Review: http://cr.openjdk.java.net/~anazarov/JDK-8184961/webrev.00/webrev/
    >>>
    >>>
    >>> —Andrei
    >
   
   

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8184961: jdk.test.lib.util.FileUtils.deleteFileWithRetry0 should wait for absence of a file

Chris Hegarty

> On 26 Jul 2017, at 18:03, Hohensee, Paul <[hidden email]> wrote:
>
> A double negative (!Files.notExists) somewhat unusual coding and will perplex people reading it. They might even switch it back to Files.exists as a style cleanup, so I recommend adding a comment explaining why you’re using it.

As the original author of this code, +1000

-Chris.


> Thanks,
>
> Paul
>
> On 7/25/17, 5:08 PM, "core-libs-dev on behalf of Andrey Nazarov" <[hidden email] on behalf of [hidden email]> wrote:
>
>    Thanks, Brian
>
>    —Andrei
>> On 25 Jul 2017, at 16:41, Brian Burkhalter <[hidden email]> wrote:
>>
>> Hi Andrei,
>>
>> I think this looks good.
>>
>> Thanks,
>>
>> Brian
>>
>>> On Jul 25, 2017, at 3:59 PM, Andrey Nazarov <[hidden email]> wrote:
>>>
>>> Can anyone look?
>>>
>>> —Thanks,
>>> Andrei
>>>> On 19 Jul 2017, at 18:21, Andrey Nazarov <[hidden email]> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Could you review fix in test library which should help to resolve intermittent issues on Windows machines during directories clean up.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8184961 <https://bugs.openjdk.java.net/browse/JDK-8184961>
>>>> Review: http://cr.openjdk.java.net/~anazarov/JDK-8184961/webrev.00/webrev/
>>>>
>>>>
>>>> —Andrei
>>
>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8184961: jdk.test.lib.util.FileUtils.deleteFileWithRetry0 should wait for absence of a file

Andrey Nazarov

> On 26 Jul 2017, at 12:31, Chris Hegarty <[hidden email]> wrote:
>
>
>> On 26 Jul 2017, at 18:03, Hohensee, Paul <[hidden email]> wrote:
>>
>> A double negative (!Files.notExists) somewhat unusual coding and will perplex people reading it. They might even switch it back to Files.exists as a style cleanup, so I recommend adding a comment explaining why you’re using it.
>
> As the original author of this code, +1000
Code has been pushed. I’ll create new issue for that :)

—Andrei

>
> -Chris.
>
>
>> Thanks,
>>
>> Paul
>>
>> On 7/25/17, 5:08 PM, "core-libs-dev on behalf of Andrey Nazarov" <[hidden email] on behalf of [hidden email]> wrote:
>>
>>   Thanks, Brian
>>
>>   —Andrei
>>> On 25 Jul 2017, at 16:41, Brian Burkhalter <[hidden email]> wrote:
>>>
>>> Hi Andrei,
>>>
>>> I think this looks good.
>>>
>>> Thanks,
>>>
>>> Brian
>>>
>>>> On Jul 25, 2017, at 3:59 PM, Andrey Nazarov <[hidden email]> wrote:
>>>>
>>>> Can anyone look?
>>>>
>>>> —Thanks,
>>>> Andrei
>>>>> On 19 Jul 2017, at 18:21, Andrey Nazarov <[hidden email]> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> Could you review fix in test library which should help to resolve intermittent issues on Windows machines during directories clean up.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8184961 <https://bugs.openjdk.java.net/browse/JDK-8184961>
>>>>> Review: http://cr.openjdk.java.net/~anazarov/JDK-8184961/webrev.00/webrev/
>>>>>
>>>>>
>>>>> —Andrei
>>>
>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8184961: jdk.test.lib.util.FileUtils.deleteFileWithRetry0 should wait for absence of a file

Andrey Nazarov
Hi,

Please review this simple documentation patch

diff -r a133a7d1007b test/lib/jdk/test/lib/util/FileUtils.java
--- a/test/lib/jdk/test/lib/util/FileUtils.java Tue Jul 25 17:04:46 2017 -0700
+++ b/test/lib/jdk/test/lib/util/FileUtils.java Wed Jul 26 13:20:44 2017 -0700
@@ -98,6 +98,7 @@
         while (true) {
             try {
                 Files.delete(path);
+                // Checks for absence of the file. Semantics of Files.exists() is not the same.
                 while (!Files.notExists(path)) {
                     times++;
                     if (times > MAX_RETRY_DELETE_TIMES) {


—Andrei

> On 26 Jul 2017, at 12:45, Andrey Nazarov <[hidden email]> wrote:
>
>
>> On 26 Jul 2017, at 12:31, Chris Hegarty <[hidden email]> wrote:
>>
>>
>>> On 26 Jul 2017, at 18:03, Hohensee, Paul <[hidden email]> wrote:
>>>
>>> A double negative (!Files.notExists) somewhat unusual coding and will perplex people reading it. They might even switch it back to Files.exists as a style cleanup, so I recommend adding a comment explaining why you’re using it.
>>
>> As the original author of this code, +1000
> Code has been pushed. I’ll create new issue for that :)
>
> —Andrei
>>
>> -Chris.
>>
>>
>>> Thanks,
>>>
>>> Paul
>>>
>>> On 7/25/17, 5:08 PM, "core-libs-dev on behalf of Andrey Nazarov" <[hidden email] on behalf of [hidden email]> wrote:
>>>
>>>  Thanks, Brian
>>>
>>>  —Andrei
>>>> On 25 Jul 2017, at 16:41, Brian Burkhalter <[hidden email]> wrote:
>>>>
>>>> Hi Andrei,
>>>>
>>>> I think this looks good.
>>>>
>>>> Thanks,
>>>>
>>>> Brian
>>>>
>>>>> On Jul 25, 2017, at 3:59 PM, Andrey Nazarov <[hidden email]> wrote:
>>>>>
>>>>> Can anyone look?
>>>>>
>>>>> —Thanks,
>>>>> Andrei
>>>>>> On 19 Jul 2017, at 18:21, Andrey Nazarov <[hidden email]> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Could you review fix in test library which should help to resolve intermittent issues on Windows machines during directories clean up.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8184961 <https://bugs.openjdk.java.net/browse/JDK-8184961>
>>>>>> Review: http://cr.openjdk.java.net/~anazarov/JDK-8184961/webrev.00/webrev/
>>>>>>
>>>>>>
>>>>>> —Andrei
>>>>
>>>
>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8184961: jdk.test.lib.util.FileUtils.deleteFileWithRetry0 should wait for absence of a file

Chris Hegarty

> On 26 Jul 2017, at 21:22, Andrey Nazarov <[hidden email]> wrote:
>
> Hi,
>
> Please review this simple documentation patch
>
> diff -r a133a7d1007b test/lib/jdk/test/lib/util/FileUtils.java
> --- a/test/lib/jdk/test/lib/util/FileUtils.java Tue Jul 25 17:04:46 2017 -0700
> +++ b/test/lib/jdk/test/lib/util/FileUtils.java Wed Jul 26 13:20:44 2017 -0700
> @@ -98,6 +98,7 @@
>         while (true) {
>             try {
>                 Files.delete(path);
> +                // Checks for absence of the file. Semantics of Files.exists() is not the same.
>                 while (!Files.notExists(path)) {
>                     times++;
>                     if (times > MAX_RETRY_DELETE_TIMES) {

Thank you. Reviewed.

-Chris.
Loading...