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
