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

[Dmitry Samersoff]

Serguei,

http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/

I refactored the debugInit_exit function.

Now we have separate exit code for transport error and better readable
code.

-Dmitry


On 2014-08-27 00:22, serguei.spitsyn at oracle.com wrote:
> Dmitry,
> 
> It makes sense to consider a special exit code for transport init error.
> Other than that it looks good.
> No need to re-review.
> 
> Thanks,
> Serguei
> 
> On 8/26/14 10:46 AM, Dmitry Samersoff wrote:
>> 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.