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

[Coleen Phillimore]

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.

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
>>>>>>>>
>>>>>>>
>>>