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

[Vladimir Kempik]

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