RFR: 10: 8177522: -XX:OnOutOfMemoryError does not work if supplied twice on windows

harold seigel harold.seigel at oracle.com
Fri Apr 28 11:54:31 UTC 2017


Hi Vladimir,

Other than minor nit of updating a few copyright dates, your fix looks good.

Thanks, Harold


On 4/28/2017 5:13 AM, Vladimir Kempik wrote:
> Hello everyone, can I have one more review for this please ?
>
>
> Thanks, Vladimir
> 27.04.17 15:16, David Holmes пишет:
>> On 27/04/2017 6:37 PM, Vladimir Kempik wrote:
>>> Hello
>>>
>>> Thanks for looking into this.
>>>
>>> Updated webrev: http://cr.openjdk.java.net/~vkempik/8177522/webrev.01
>>
>> Looks good.
>>
>>> see answers below
>>
>> Response below ...
>>
>>> 27.04.17 10:39, David Holmes пишет:
>>>> Hi Vladimir,
>>>>
>>>> Thanks for fixing this.
>>>>
>>>> On 26/04/2017 7:58 PM, Vladimir Kempik wrote:
>>>>> Hello
>>>>>
>>>>> Please review this fix for bug JDK-8177522
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8177522>
>>>>>
>>>>> The issue is windows only.
>>>>>
>>>>> When two OnOutOfMemoryError options are passed to java, they are
>>>>> combined using '\n' as separator.
>>>>>
>>>>> But when OnError parses the string, it only knows ';' as separator of
>>>>> commands.
>>>>>
>>>>> On nix systems it works fine because the command with '\n' in it are
>>>>> passed to shell where it executed fine.
>>>>>
>>>>> With this patch I'm making windows routine to behave similary to nix,
>>>>> passing command to cmd.exe for execution, I just have to replace all
>>>>> '\n' with '&'.
>>>>
>>>> Overall this approach looks good to me.
>>>>
>>>> os_windows.cpp:
>>>>
>>>> +   char * cmd_prefix = "cmd /C ";
>>>>
>>>> Should that be cmd.exe as in vmError.cpp?
>>> I have made both vmerror and here as "cmd /C" now , cmd and cmd.exe is
>>> the same thing for windows anyway.
>>>>
>>>> Two nits:
>>>>
>>>> + // now replace all '\n' with '&'
>>>>
>>>> Comment should be indented 2 spaces.
>>>>
>>>> +   while ((substring = strchr (substring, '\n')) != NULL) {
>>>>
>>>> Remove space after strchr.
>>>>
>>> done
>>>> ---
>>>>
>>>> vmError.cpp:
>>>>
>>>> I'm wondering though, do we need to modify the vmError code given we
>>>> will add the "cmd.exe /C" prefix in fork_and_exec anyway?
>>> I thought it should be similar to other platforms, indicating we are
>>> launching the command thru the shell/cmd, having consistency.
>>
>> Must admit I'm not sure why the other platforms use a shell to run 
>> the command ... maybe to allow for the command to be a script.
>>
>> Treating the two bits of code independently is not unreasonable. My 
>> only concern is that we will use additional memory/resources doing:
>>
>> cmd /C cmd /C <real cmd>
>>
>> I have some reservations about this, but we have plenty of time to 
>> see how this works out.
>>
>> Thanks,
>> David
>>
>>>>
>>>> ---
>>>>
>>>> test/runtime/ErrorHandling/TestOnOutOfMemoryError.java
>>>>
>>>> The test should test both a single -XX:OnOutOfMemoryError, and
>>>> multiple ones.
>>>>
>>> Done
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>> Thanks, Vladimir.
>>>>> Such workaround is needed to not break compatibility with existing
>>>>> systems.
>>>>>
>>>>> Testing: jprt, included testcase.
>>>>>
>>>>> Webrev - http://cr.openjdk.java.net/~vkempik/8177522/webrev.00/
>>>>>
>>>>> Bug - https://bugs.openjdk.java.net/browse/JDK-8177522
>>>>>
>>>>> Thanks, Vladimir
>>>>>
>>>
>



More information about the hotspot-runtime-dev mailing list