RFR: JDK-8158066: SourceDebugExtensionTest fails to rename file

[Gary Adams]

Looking through the test history for the vm/mlvm version of 
InstallSDE.java there has never
been a reported problem with the renameTo operation. I'm inclined to not 
apply the change
made to the com/sun/jdi/sde test until the failure is observed.

I'm satisfied with the testing at this point and could use another 
reviewer before
pushing.

On 1/11/19, 6:14 PM, David Holmes wrote:
> Hi Gary,
>
> As I mentioned earlier there are two versions of InstallSDE.java in 
> the tests, so both should be updated.
>
> Your strategy to avoid the windows delete issue seems reasonable.
>
> David
>
> On 12/01/2019 4:53 am, Gary Adams wrote:
>> Here's a webrev for review purposes.
>> No failures after ~1000 testruns.
>>
>>    Webrev: http://cr.openjdk.java.net/~gadams/8158066/webrev.00/
>>    Issue: https://bugs.openjdk.java.net/browse/JDK-8158066
>>
>> On 1/11/19, 11:01 AM, Gary Adams wrote:
>>> I've been reading recently that Windows delete file operations
>>> can be delayed, if there are open handles against the file.
>>> Others have reported virus checkers or other indexing
>>> programs may hold a handle on a file preventing the
>>> deletion being completed.
>>>
>>> The basic logic in InstallSDE is
>>>    A + B = C
>>>    delete A
>>>    rename C to A
>>>
>>> I think we can work around the issue with
>>>   rename A to A1
>>>   A1 + B = C
>>>   delete A1
>>>   rename C to A
>>>
>>> diff --git a/test/jdk/com/sun/jdi/sde/InstallSDE.java 
>>> b/test/jdk/com/sun/jdi/sde/InstallSDE.java
>>> --- a/test/jdk/com/sun/jdi/sde/InstallSDE.java
>>> +++ b/test/jdk/com/sun/jdi/sde/InstallSDE.java
>>> @@ -31,9 +31,17 @@
>>>      }
>>>
>>>      static void install(File inOutClassFile, File attrFile) throws 
>>> IOException {
>>> -        File tmpFile = new File(inOutClassFile.getPath() + "tmp");
>>> -        new InstallSDE(inOutClassFile, attrFile, tmpFile);
>>> -        if (!inOutClassFile.delete()) {
>>> +        File tmpFile = new File(inOutClassFile.getPath() + "tmp-out");
>>> +        File tmpInOutClassFile = new File(inOutClassFile.getPath() 
>>> + "tmp-in");
>>> +
>>> +        // Workaround delayed file deletes on Windows using a tmp 
>>> file name
>>> +        if (!inOutClassFile.renameTo(tmpInOutClassFile)) {
>>> +            throw new IOException("tmp copy of inOutClassFile 
>>> failed");
>>> +        }
>>> +
>>> +        new InstallSDE(tmpInOutClassFile, attrFile, tmpFile);
>>> +
>>> +        if (!tmpInOutClassFile.delete()) {
>>>              throw new IOException("inOutClassFile.delete() failed");
>>>          }
>>>          if (!tmpFile.renameTo(inOutClassFile)) {
>>>
>>>
>>> Testing in progress ...
>>>
>>> On 1/11/19, 7:40 AM, David Holmes wrote:
>>>> Hi Gary,
>>>>
>>>> On 11/01/2019 9:22 pm, gary.adams at oracle.com wrote:
>>>>> After ~1000 testruns I hit a failure in MangleTest and a jtreg 
>>>>> agent communication issue with SourceDebugExtension.
>>>>>
>>>>> https://java.se.oracle.com:10065/mdash/builds/2019-01-10-1159535.gary.adams.jdk-jdb/results?search=status%3Afailed%20AND%20-state%3Ainvalid 
>>>>>
>>>>>
>>>>>
>>>>> *java.io.IOException: tmpFile.renameTo(inOutClassFile) failed*
>>>>>     at InstallSDE.install(InstallSDE.java:40)
>>>>>     at MangleTest.testSetUp(MangleTest.java:38)
>>>>>     at MangleTest.main(MangleTest.java:31)
>>>>
>>>> You might want to add some code in:
>>>>
>>>> ./java.base/windows/native/libjava/WinNTFileSystem_md.c:Java_java_io_WinNTFileSystem_rename0 
>>>>
>>>>
>>>> that checks the result of _wrename to see what error is occurring. 
>>>> I suspect it will be EACCES which won't be that helpful:
>>>>
>>>> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/rename-wrename?view=vs-2017 
>>>>
>>>>
>>>> but it would be good to confirm.
>>>>
>>>>>
>>>>> result: Error. Agent communication error: 
>>>>> java.io.IOException:*Agent: unexpected op: 71;*  check console log 
>>>>> for any additional details
>>>>>
>>>>> I added some additional print outs for the rename issue and have 
>>>>> run ~2000 additional
>>>>> testruns with no test failures repeated, yet. But there were 2 
>>>>> mach5 reported errors:
>>>>>
>>>>>     TimeoutException in CLEANUP.
>>>>
>>>> That's a mach5 issue.
>>>>
>>>> David
>>>>
>>>>>
>>>>> I'll do some more investigation with making the rename operation 
>>>>> more robust
>>>>> before removing SourceDebugExtensionTest from the problem list.
>>>>>
>>>>> On 1/10/19 8:28 PM, David Holmes wrote:
>>>>>> Hi Gary,
>>>>>>
>>>>>> On 10/01/2019 11:02 pm, gary.adams at oracle.com wrote:
>>>>>>> SourceDebugExtensionTest was placed on the ProblemList
>>>>>>> because of filesystem rename issues on Windows 2012 test systems.
>>>>>>> The test does not appear to fail on the current mach5 test systems.
>>>>>>> There was some question concerning symbolic links that might have
>>>>>>> been problematic at that time. Additional testing in progress...
>>>>>>
>>>>>> Also note that there are two sets of SDE related tests and two 
>>>>>> versions of the InstallSDE.java file that both use renameTo. In 
>>>>>> nsk the testcase seems to be:
>>>>>>
>>>>>> ./hotspot/jtreg/vmTestbase/vm/mlvm/share/StratumClassesBuilder.java
>>>>>>
>>>>>> and AFAICS it is not excluded. So removing the exclusion seems 
>>>>>> reasonable to me. It was most like a concurrency issue with the 
>>>>>> virus scanner.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>
>>>>>>>    Issue: https://bugs.openjdk.java.net/browse/JDK-8158066
>>>>>>>
>>>>>>> diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt
>>>>>>> --- a/test/jdk/ProblemList.txt
>>>>>>> +++ b/test/jdk/ProblemList.txt
>>>>>>> @@ -838,8 +838,6 @@
>>>>>>>
>>>>>>>   com/sun/jdi/RepStep.java 8043571 generic-all
>>>>>>>
>>>>>>> -com/sun/jdi/sde/SourceDebugExtensionTest.java 8158066 windows-all
>>>>>>> -
>>>>>>>   com/sun/jdi/NashornPopFrameTest.java 8187143 generic-all
>>>>>>>
>>>>>
>>>
>>