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

[Ioi Lam]

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.

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