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

[David Holmes]

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
>