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

[serguei.spitsyn at oracle.com]

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