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

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