RFR 8217347 [TESTBUG] runtime/appcds/jvmti/InstrumentationTest.java timed out

[Ioi Lam]

Hi David, thanks for the review!

- Ioi

On 3/26/19 12:13 AM, David Holmes wrote:
> Hi Ioi,
>
> On 26/03/2019 10:37 am, Ioi Lam wrote:
>> Hi David,
>>
>> http://cr.openjdk.java.net/~iklam/jdk13/8217347-appcds-InstrumentationTest-timeout.v02/ 
>>
>>
>> Thanks for the explanation. I've reverted my previous changes and 
>> instead modified how the hand-shake is done. There's no need to use 
>> VirtualMachine.list() as the pid is passed as part of the hand-shake.
>>
>> // Hand-shake protocol with the child process
>> // [1] Parent process (this process) launches child process 
>> (InstrumentationApp)
>> //     and then waits until child process writes its pid into the 
>> flagFile.
>> // [2] Child process process starts up, writes its pid into the 
>> flagFile,
>> //     and waits for the flagFile to be deleted.
>> // [3] When parent process gets the pid, it attaches to the child 
>> process
>> //     (if we attempt to attach to a process too early, the SIGQUIT
>> //     may cause the child to die) and deletes the flagFile.
>> // [4] Child process resumes execution.
>
> Okay that all seems fine.
>
> The test could be simplified further I think but that's a different 
> story.
>
> Thanks,
> David
>
>> Thanks
>> - Ioi
>>
>>
>> On 3/24/19 10:28 PM, David Holmes wrote:
>>> On 25/03/2019 3:06 pm, Ioi Lam wrote:
>>>>
>>>>
>>>> On 3/24/19 9:14 PM, David Holmes wrote:
>>>>> Hi Ioi,
>>>>>
>>>>> On 25/03/2019 1:46 pm, Ioi Lam wrote:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8217347
>>>>>> http://cr.openjdk.java.net/~iklam/jdk13/8217347-appcds-InstrumentationTest-timeout.v01/ 
>>>>>>
>>>>>>
>>>>>> This test case used VirtualMachine.list() to find the pid of the 
>>>>>> the child
>>>>>> process. However, as David Holmes commented in the bug report, it's
>>>>>> suspected that this call may lead to timeout.
>>>>>>
>>>>>> In any case, since Process.pid() has been added since JDK 9, it's 
>>>>>> better to call this
>>>>>> API directly. That way, we can avoid unnecessary dependency on 
>>>>>> VirtualMachine.list().
>>>>>
>>>>> In so far as I can see how you replaced the VM.list() with 
>>>>> Process.pid() this seems okay. I'm having trouble actually 
>>>>> understanding the handshake being used to control things though. I 
>>>>> would have expected the target VM to create the flagfile to 
>>>>> indicate it is ready for attaching, then it would wait for the 
>>>>> flagfile to be deleted.
>>>>>
>>>>
>>>> Hi David,
>>>>
>>>> As far as I can tell, the handshake is this: the main test process 
>>>> creates the flag file and passes the name of the flag file to the 
>>>> child process (whose main class is InstrumentationApp). The child 
>>>> process will wait until the main process deletes the flag file.
>>>>
>>>> After the child process is launched, the main process attaches to 
>>>> it (using the child's pid), and then deletes the flag file. At that 
>>>> point, the child will resume execution.
>>>
>>> Okay but the problem is that the main process can try to attach to 
>>> the child process before the child is ready to be attached to. This 
>>> is a day one limitation of the attach-on-demand process. If the 
>>> child process created the flags file (using the name supplied by the 
>>> main process) the main process could wait until it sees it to do the 
>>> attach.
>>>
>>> Anyway this is not directly related to your fix, just be aware this 
>>> test can fail in obscure ways as the child process may silently 
>>> disappear if the SIGQUIT used to attach-on-demand arrives too soon.
>>>
>>>>
>>>> BTW, I found this comment to be out-dated so I changed it:
>>>>
>>>> -        // At this point, the child process is not yet launched. 
>>>> Note that
>>>> -        // TestCommon.exec() and OutputAnalyzer.OutputAnalyzer() 
>>>> both block
>>>> -        // until the child process has finished.
>>>> -        //
>>>> -        // So, we will launch a AgentAttachThread which will poll 
>>>> the system
>>>> -        // until the child process is launched, and then do the 
>>>> attachment.
>>>> -        // The child process is uniquely identified by having 
>>>> flagFile in its
>>>> -        // command-line -- see AgentAttachThread.getPid().
>>>>
>>>> +        // At this point, the child process is not yet launched.
>>>> +        // We will launch a AgentAttachThread which will wait until
>>>> +        // the child process's pid is known, and then do the 
>>>> attachment.
>>>
>>> Okay but the comment is out of date because you now return the 
>>> process directly, which means you no longer need the 
>>> AgentAttachThread at all, you can do the attach logic directly from 
>>> the main thread.
>>>
>>> Cheers,
>>> David
>>> -----
>>>
>>>>           AgentAttachThread t = new AgentAttachThread(flagFile, 
>>>> agentJar);
>>>>           t.start();
>>>>           return t;
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Tested with tiers{1,2,3}. Also removed some unnecessary @module 
>>>>>> and import lines.
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>
>>