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

[David Holmes]

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
>>>
>