[rfc][icedtea-web] replace runtime().exec by processbuilder

[Andrew Azores]

On 15/06/15 12:15 PM, Jiri Vanek wrote:
> On 06/15/2015 06:11 PM, Andrew Azores wrote:
>> On 15/06/15 11:57 AM, Jiri Vanek wrote:
>>> On 06/15/2015 05:46 PM, Andrew Azores wrote:
>>>> On 15/06/15 11:34 AM, Jiri Vanek wrote:
>>>>> ssia
>>>>>
>>>>>
>>>>> I'm also in temptation to push this to 1.6
>>>>>
>>>>> The handling of streams is just terrible :(
>>>>>
>>>>>
>>>>> J.
>>>>
>>>>> -            Process p = Runtime.getRuntime().exec(command);
>>>>> -            new StreamEater(p.getErrorStream()).start();
>>>>> -            new StreamEater(p.getInputStream()).start();
>>>>> -            p.getOutputStream().close();
>>>>> -
>>>>> +            ProcessBuilder pb = new ProcessBuilder(command);
>>>>> +            pb.inheritIO();
>>>>> +            Process p =pb.start();
>>>>> +            try {
>>>>> +                p.waitFor();
>>>>> +            } catch (InterruptedException e) {
>>>>> + 
>>>>> OutputController.getLogger().log(OutputController.Level.ERROR_ALL, 
>>>>> e);
>>>>> +            }
>>>>
>>>> Where does the "waitFor" come from? I'm not seeing that the old 
>>>> code does the same thing (but maybe
>>>
>>> Surprisingly no - the change of behavior   is to not include it.
>>> The wait is hidden in the handling of streams.
>>> As streaminheritance is different, then the waitFor is the 
>>> necessarything to do.
>>>
>>
>> Ah alright. I figured that might have been what was happening but it 
>> wasn't clear even when reading
>> the StreamEater that it sort of implicitly caused a waitFor effect. 
>> This way is much, much better then!
>>
>>>
>>>> I'm just missing it and it does). Is this meant to be a change in 
>>>> behaviour? If that
>>>> InterruptedException does get thrown, is it ok that the current 
>>>> thread continues on in parallel with
>>>> process p?
>>>
>>> For that I dont have answer. Whats your suggestion? HAve you ever 
>>> seenthis exception? :)
>>>
>>> J
>>>
>>
>> Well, only when I've been intentionally trying to interrupt and wake 
>> up a waiting thread. But
>
> yes:)
>> spurious wakeups [0] can happen and are probably worth handling. In 
>> this case I suppose that just
> hmmm
>
>> means putting the waitFor into a loop and continuing to waitFor until 
>> p.exitStatus() returns rather
>> than throws IllegalThreadStateException? Sounds pretty messy :(
>>
>> [0] https://en.wikipedia.org/wiki/Spurious_wakeup
>
>
>             Process p =pb.start();
> boolean workaround = true;
> do{
>             try {
>                p.waitFor();
> woraround=false;
>            } catch (InterruptedException e) {
> OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
>       }
> }while(workaround);
>
> ??

I don't think this has quite the right behaviour. This will break the 
loop as soon as waitFor returns, but it's apparently possible that a 
spurious wakeup can occur and the waiting thread to resume without an 
InterruptedException being generated. In that case, we'll stop waiting 
for p even though it's not actually done yet. This is why I suggested 
using the exitValue of p instead, since that can be more reliably used 
to determine if p has actually exited.

>
> ISnt better jsut keep going or die?
>

I don't know about "keep going" and what that would actually mean in 
this case and what kind of behaviour would arise. "Die" sounds 
reasonable, though, but then we need to check if p is actually finished 
after we wake up and then decide to die if it isn't - so then why not 
instead of dying just waitFor p again?

-- 
Thanks,

Andrew Azores