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

[Jiri Vanek]

On 06/15/2015 06:23 PM, Andrew Azores wrote:
> 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?


May you provide code snippet please?

Is this kind of awaking really worthy of surviving?  I'm really inclined to just catch, log, and 
continue ITW running...

J.