[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR 8024055: serviceability/attach/AttachWithStalePidFile.java createJavaPidFile() fails
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2015-03-24 9:55:09
Message-ID: 5511347D.7050700 () oracle ! com
[Download RAW message or body]

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.spitsyn@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.bachorik@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-
>>>>
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]