Thank you for explanation, Jaroslav!
Would it make sense to add short comment before the loop?
No need to re-review.
Thanks,
Serguei
On 3/24/15 2:44 AM, Jaroslav Bachorik wrote:
Hi Serguei,
On 24.3.2015 10:22, serguei.spit...@oracle.com wrote:
Jaroslav,
test/serviceability/attach/AttachWithStalePidFile.java
A question:
101 private static void waitForTargetReady(Process target) throws
IOException {
102 BufferedReader br = new BufferedReader(new
InputStreamReader(target.getInputStream()));
103 String line = br.readLine();
104 while (line != null &&
!line.equals(AttachWithStalePidFileTarget.READY_MSG)) {
105 line = br.readLine();
106 }
107 // target VM ready
108 }
Not sure, in what condition the br.readLine() returns null.
Will null be returned if the target thread did not write anything
to the stdout yet?
No, NULL will be returned only in case of EOF (eg. stream has been
closed). Otherwise the readLine() will block until some data is
available.
The while loop will complete if line == null.
Do we really want this condition? :
104 while (line == null ||
!line.equals(AttachWithStalePidFileTarget.READY_MSG)) {
The crucial part is the comparison with the READY_MSG which indicates
that the target VM has actually passed the initialization and is ready
for the test. The 'line != null' is necessary to prevent NPE being
thrown. While the test would fail anyway it is more informational to
fail while trying to attach than an NPE.
-JB-
Otherwise, it looks good.
Thanks,
Serguei
On 3/23/15 12:20 PM, Jaroslav Bachorik wrote:
On 23.3.2015 13:44, Staffan Larsen wrote:
Looks good, but please print the exception at line 118 in
AttachWithStalePidFile.java.
Hm, like this http://cr.openjdk.java.net/~jbachorik/8024055/webrev.01 ?
-JB-
Thanks,
/Staffan
On 23 mar 2015, at 12:42, Jaroslav Bachorik
<jaroslav.bacho...@oracle.com> wrote:
Please, review the following test change
Issue : https://bugs.openjdk.java.net/browse/JDK-8024055
Webrev: http://cr.openjdk.java.net/~jbachorik/8024055/webrev.00
This request is a follow-up to the stalled review request
http://mail.openjdk.java.net/pipermail/serviceability-dev/2014-October/015785.html
(the issue has changed its owner since then)
As stated in the original request:
"
This patch fixes two intermittent issues seen over the past year:
a) Possible failure where an existing pid-file is not owned by the
test user
b) Race during startup where we try to attach to the target before
it’s ready (removed arbitrary 5sec sleep)
"
This version is addressing David's comment about better processing
the target process' stdout directly and not asynchronously.
Thanks,
-JB-
