RFR(xs): 8080925: Make error log write timeout parameter configurable

[Coleen Phillimore]

On 6/25/15 7:50 AM, Coleen Phillimore wrote:
>
> My preference is to not add the test.   You verified it manually and 
> it's in an area of code that isn't likely to accidentally change so 
> needs a regression test every day.

edit: so _doesn't_ need a regression test every day.
Coleen
>
> I'd be happy to sponsor the revision without the test.
> Coleen
>
> On 6/25/15 2:41 AM, Thomas Stüfe wrote:
>> Hi Christian, David,
>>
>> yes, I admit you are right. We run similar tests since a long time at 
>> SAP,
>> and even with a lot of leeway we run into situations where the VM 
>> does not
>> finish in time. Very rarely (maybe once a month) but still. 
>> Especially on
>> older hardware, like AS/400. In almost all situations these were false
>> positives, only once this was a legitimate error.
>>
>> My suggestions would be
>>
>> 1) Either I could just not test. The change is arguably easy enough 
>> to omit
>> a specific test.
>> 2) Or I could add a change (which would be a good idea anyway) to, 
>> before I
>> abort, print out a last line into the hs-err file: "-- Timeout During 
>> Error
>> Reporting after xxx seconds --". In the test, instead of measuring 
>> time, I
>> would scan for this trailing line in the hs-err file. That would have 
>> the
>> additional advantage that we later know why the hs-err file is torn.
>>
>> I also could do (1) now, and (2) in a separate patch - I have vacations
>> coming up in a week and would like to finish this beforehand.
>>
>> Thomas
>>
>>
>> On Thu, Jun 25, 2015 at 7:34 AM, David Holmes <david.holmes at oracle.com>
>> wrote:
>>
>>> Hi Thomas,
>>>
>>> On 25/06/2015 2:01 AM, Christian Tornqvist wrote:
>>>
>>>> Hi Thomas,
>>>>
>>>> I'm concerned with the time measurement. We run a lot of our tests
>>>> concurrently on a wide spread of hardware, I wouldn't be surprised 
>>>> if it
>>>> sometimes took more than 4s to get the JVM to a state where the
>>>> ErrorHandlerTests are invoked.
>>>>
>>> I share Christian's concern, particularly as you start measuring before
>>> even exec'ing the new JVM:
>>>
>>>    70     long t1 = System.currentTimeMillis();
>>>    71
>>>    72     OutputAnalyzer output_detail = new 
>>> OutputAnalyzer(pb.start());
>>>
>>> There's really no reliable way to validate a timeout using an external
>>> observer: you either have so much leeway that you won't spot an 
>>> unexpected
>>> delay; or too little such that some platforms/devices don't make it. 
>>> The
>>> framework timeout will at least detect if the underlying VM 
>>> execution fails
>>> to terminate at all.
>>>
>>> Sorry I don't have any better suggestions for testing this.
>>>
>>> David
>>> -----
>>>
>>>
>>>
>>>   Thanks,
>>>> Christian
>>>>
>>>> -----Original Message-----
>>>> From: hotspot-runtime-dev [mailto:
>>>> hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of Coleen
>>>> Phillimore
>>>> Sent: Wednesday, June 24, 2015 11:45 AM
>>>> To: hotspot-runtime-dev at openjdk.java.net
>>>> Subject: Re: RFR(xs): 8080925: Make error log write timeout parameter
>>>> configurable
>>>>
>>>>
>>>> Hi Thomas,
>>>>
>>>> This change looks okay, I guess.  I've been avoiding tests that 
>>>> crash the
>>>> JVM because I don't know if this will mess up test execution in any
>>>> way.   Also I guess I don't see any other way to add the test 
>>>> without a
>>>> command line parameter, like TestSafeFetchInErrorHandler , but I feel
>>>> like we're swamped in command line parameters!   But you just got 
>>>> rid of
>>>> one that was broken, so I guess this is fine.
>>>>
>>>> I can sponsor your change if someone from our SQE group okays the 
>>>> test.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>> On 6/24/15 11:30 AM, Thomas Stüfe wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> This change is in review limbo since a month...
>>>>>
>>>>> Could I have a second reviewer, please, and a sponsor?
>>>>>
>>>>> Or, if the change is not wanted, a reason why?
>>>>>
>>>>> Thanks a lot,
>>>>>
>>>>> Thomas
>>>>>
>>>>>
>>>>> On Mon, Jun 15, 2015 at 2:58 PM, Thomas Stüfe
>>>>> <thomas.stuefe at gmail.com>
>>>>> wrote:
>>>>>
>>>>>   Hi all,
>>>>>> may I have a second review for this tiny change? I also need a 
>>>>>> sponsor.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> On Fri, May 22, 2015 at 4:21 PM, Thomas Stüfe
>>>>>> <thomas.stuefe at gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>   Hi Staffan,
>>>>>>> thanks!
>>>>>>>
>>>>>>> ...Thomas
>>>>>>>
>>>>>>> On Fri, May 22, 2015 at 1:55 PM, Staffan Larsen <
>>>>>>> staffan.larsen at oracle.com> wrote:
>>>>>>>
>>>>>>>   Looks good to me.
>>>>>>>> A small nit in globals.hpp:925: "an timeout” -> “a timeout”. No
>>>>>>>> need for a new webrev if you fix this.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> /Staffan
>>>>>>>>
>>>>>>>>   On 22 maj 2015, at 13:33, Thomas Stüfe <thomas.stuefe at gmail.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> please review this small change:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8080925/webrev.00/webre 
>>>>>>>>>
>>>>>>>>> v/
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8080925
>>>>>>>>>
>>>>>>>>> This is a small addition which makes the timeout to write error
>>>>>>>>> log
>>>>>>>>>
>>>>>>>> files
>>>>>>>>
>>>>>>>>> configurable.
>>>>>>>>>
>>>>>>>>> Thanks & Kind Regards,
>>>>>>>>>
>>>>>>>>> Thomas
>>>>>>>>>
>>>>>>>>
>>>>
>