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

[Jiri Vanek]

On 06/15/2015 06:58 PM, Andrew Azores wrote:
> On 15/06/15 12:52 PM, Jiri Vanek wrote:
>> On 06/15/2015 06:37 PM, Andrew Azores wrote:
>>> On 15/06/15 12:26 PM, Jiri Vanek wrote:
>>>>
>>>>> 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?
>>>
>>>              ProcessBuilder pb = new ProcessBuilder(command);
>>>              pb.inheritIO();
>>>              Process p = pb.start();
>>>              boolean pTerminated = false;
>>>              while (!pTerminated) {
>>>                  try {
>>>                      p.waitFor();
>>>                  } catch (InterruptedException e) {
>>> OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
>>>                  }
>>>                  try {
>>>                      p.exitValue();
>>>                      pTerminated = true;
>>>                  } catch (IllegalThreadStateException e) {
>>> OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
>>>                  }
>>>              }
>>>
>>> Hopefully the formatting survives.
>>
>> Does :) Thanx!
>>>
>>>>
>>>> Is this kind of awaking really worthy of surviving?  I'm really inclined to just catch, log, and
>>>> continue ITW running...
>>>>
>>>> J.
>>>
>>> It's not that hard to correctly handle it, although the code does look pretty ugly. I think the
>>> better question to ask is if a spurious wakeup does occur here, will ITW continue to run properly?
>>> Because like I said, it's possible that the wakeup occurs and there is no exception to catch or log,
>>> and so you just end up with a silent failure later on.
>>
>> I really don't know. I'm still a bit more inclined to my straight original version.
>> If you insists on this one you posted, please state it (jsut for record).
>>
>> I'm probably unable to provide any more decision points:(
>>
>> J.
>
> I "gently insist". I think mine is a more correct approach to take, but it is targeting what should
> be a very rare edge case and so in practice your cleaner code should work just as well. I just think
> it's worthwhile to add the slightly dirty loop and exitValue stuff to make sure that this code hunk
> never makes us have to worry about what happens if the calling Launcher thread ends up running in
> parallel with the forked process, because that doesn't sound easy to analyze and does sound like it
> would not lead to correct results - and above all, would be extremely hard to track and reproduce.
>
Fair enough.


There is one one related waitFor in Xdesktopentry. and few more over ITW

I'm for encapsualting

 >>>  while (!pTerminated) {
 >>>                  try {
 >>>                      p.waitFor();
 >>>                  } catch (InterruptedException e) {
 >>> OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
 >>>                  }
 >>>                  try {
 >>>                      p.exitValue();
 >>>                      pTerminated = true;
 >>>                  } catch (IllegalThreadStateException e) {
 >>> OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
 >>>                  }
 >>>              }

I'm for separate it into  method waitForSafely(Process p), and use in all palces across ITW (where 
the waitFor is used, and the InterruptedException is cacth and just logged.


oook?


Thanx!

  J.