RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again

[Dmitry Samersoff]

Serguei,

exit_code set to 1 at ll. 1288

I thought of refactoring this code for better readability but finally
decide to keep changes minimal.

-Dmitry


On 2014-08-26 21:35, serguei.spitsyn at oracle.com wrote:
> Dmitry,
> 
> This looks good in general.
> But what error code will be returned in the case of
> AGENT_ERROR_TRANSPORT_INIT ?
> Is it Ok to return whatever exit_code we already have or we better
> return some special error code?
> 
> Probably, it is Ok to keep the comment at the line 1315 as it is.
> 
> 
> Thanks,
> Serguei
> 
> On 8/26/14 7:47 AM, Dmitry Samersoff wrote:
>> Staffan,
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/
>>
>> After couple of experiments I end up with simple fix - don't coredump on
>> AGENT_ERROR_TRANSPORT_INIT
>>
>> Current code don't propagate transport error to upper level, so if we
>> need fine grained control I'll rewrite transport initialization.
>>
>> -Dmitry
>>
>>
>> On 2014-08-25 15:53, Staffan Larsen wrote:
>>> On 25 aug 2014, at 13:33, Dmitry Samersoff <dmitry.samersoff at oracle.com
>>> <mailto:dmitry.samersoff at oracle.com>> wrote:
>>>
>>>> Staffan,
>>>>
>>>> On 2014-08-25 15:26, Staffan Larsen wrote:
>>>>> Dmitry,
>>>>>
>>>>> One problem with this fix is that debugInit_exit() is used from many
>>>>> places in the JDWP code. For some of these I think it still does make
>>>>> sense to receive a core dump for analysis. I agree that when parsing
>>>>> options, we do not need a core dump.
>>>> jdwp has explicit option to request a coredump at debugInit_exit().
>>>>
>>>> Is it enough or I should create a more sophisticated fix ?
>>> I don’t think that is enough. Often a problem will happen and will not
>>> be reproducible. 
>>>
>>> Maybe this code:
>>>
>>>     if ((arg.error != JDWP_ERROR(NONE)) &&
>>>         (arg.startCount == 0) &&
>>>         initOnStartup) {
>>>         EXIT_ERROR(map2jvmtiError(arg.error), "No transports initialized");
>>>     }
>>>
>>> can use some variant of EXIT_ERROR that does not call fatalError() ?
>>>
>>> /Staffan
>>>
>>>> Actually, I don't think that coredump on every call to jni_FatalError()
>>>> (see jni.cpp:726) is necessary but this hotspot code is here for 6 years
>>>> and changing it probably out of scope of this fix.
>>>>
>>>> -Dmitry
>>>>
>>>>
>>>>> /Staffan
>>>>>
>>>>>
>>>>> On 20 aug 2014, at 17:55, Dmitry Samersoff
>>>>> <dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com>> wrote:
>>>>>
>>>>>> Serguei,
>>>>>>
>>>>>> After some additional testing I changed the fix a bit. Please take
>>>>>> a look at new version.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/
>>>>>>
>>>>>> New version reports JVMTI error to stderr.
>>>>>>
>>>>>> Also jniFatalError is not referenced any more so I remove it.
>>>>>>
>>>>>> -Dmitry
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2014-08-20 12:08, serguei.spitsyn at oracle.com wrote:
>>>>>>> Ok.
>>>>>>>
>>>>>>> Thank you for the explanation! Serguei
>>>>>>>
>>>>>>> On 8/20/14 1:01 AM, Dmitry Samersoff wrote:
>>>>>>>> Serguei,
>>>>>>>>
>>>>>>>> 1. Historically JDI test-suite had no tests for failed
>>>>>>>> transport initialization behavior and invalid parameters
>>>>>>>> handling.
>>>>>>>>
>>>>>>>> 2. As a part of JDWP hardening work I added couple of such
>>>>>>>> tests to OptionTest.java - these tests pass invalid parameters
>>>>>>>> to dt_socket transport to make sure that transport doesn't
>>>>>>>> crash (one such crash was discovered and fixed) but just return
>>>>>>>> non-zero exit code to upper level.
>>>>>>>>
>>>>>>>> 3. After fix for JDK-6694099 any non-zero exit code from
>>>>>>>> transport cause VM to coredump. Dumping multiple cores on busy
>>>>>>>> machine takes a time so harness kills the test by timeout.
>>>>>>>>
>>>>>>>> We can just increase timeout for this test but I don't think
>>>>>>>> it's a good idea to dump core when invalid parameters passed to
>>>>>>>> transport.
>>>>>>>>
>>>>>>>> So there is the fix.
>>>>>>>>
>>>>>>>> 4. After the fix tests for negative parameters will return
>>>>>>>> non-zero exit code as expected but will not dump the core.
>>>>>>>>
>>>>>>>> -Dmitry
>>>>>>>>
>>>>>>>> On 2014-08-20 00:54, serguei.spitsyn at oracle.com wrote:
>>>>>>>>> Hi Dmitry,
>>>>>>>>>
>>>>>>>>> The fix seems to be Ok. Just want to make it clear... This
>>>>>>>>> fix just changes the bug pattern. It a case of incorrect
>>>>>>>>> transport parameters the test is still going to fail but
>>>>>>>>> without crash, right?
>>>>>>>>>
>>>>>>>>> Thanks, Serguei
>>>>>>>>>
>>>>>>>>> On 8/19/14 12:09 PM, Dmitry Samersoff wrote:
>>>>>>>>>> Hi Everybody,
>>>>>>>>>>
>>>>>>>>>> Please review the fix:
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>> JDWP call jniFatalError if transport can't be initialized (e.g. wrong
>>>>>>>>>> parameters) and jniFatalError call os::abort().  Therefor
>>>>>>>>>> all transport initialization errors cause vm to coredump.
>>>>>>>>>>
>>>>>>>>>> I see no reason for debugInit_exit to call jniFatalError so
>>>>>>>>>> remove this code.
>>>>>>>>>>
>>>>>>>>>> -Dmitry
>>>>>>>>>>
>>>>>>
>>>>>> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg,
>>>>>> Russia * I would love to change the world, but they won't give me
>>>>>> the sources.
>>>>
>>>> -- 
>>>> Dmitry Samersoff
>>>> Oracle Java development team, Saint Petersburg, Russia
>>>> * I would love to change the world, but they won't give me the sources.
>>
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.