[rfc][icedtea-web] policytool in itweb-settings

[Jacob Wisor]

On 01/21/2014 08:09 PM, Andrew Azores wrote:
> On 01/21/2014 11:54 AM, Jacob Wisor wrote:
>> On 01/21/2014 12:25 AM, Jiri Vanek wrote:
>>> On 01/20/2014 09:11 PM, Andrew Azores wrote:
>>>> On 01/20/2014 02:50 PM, Andrew Azores wrote:
>>>>> On 01/20/2014 07:27 AM, Jiri Vanek wrote:
>>>>>> On 01/17/2014 10:08 PM, Andrew Azores wrote:
>>>>>>> (snip)
>>>>>>
>>>>>> [...]
>>>>>
>>>>> [...]
>>>>
>>>> [...]
>>>
>>> Only one *the* nit :)
>>>
>>>
>>>>       /**
>>>> +     * We do this so we can asynchronously monitor the exit status of the
>>>> policytool process.
>>>> +     * If it fails immediately and we need to fall back to using
>>>> reflectivePolicyToolLaunch,
>>>> +     * then the Thread spawned here is very short lived. However, if the
>>>> policytool process
>>>> +     * successfully launches, then it takes some unknown amount of time
>>>> before it closes
>>>> +     * again. We do not want to call waitFor() on the ptool process in the
>>>> main thread
>>>> +     * because this will cause the itweb-settings window to lock up.
>>>> +     * @param frame a {@link JFrame} to act as parent to warning dialogs
>>>> which may appear
>>>> +     * @param filePath a {@link String} representing the path to the file to
>>>> be opened
>>>> +     * @throws IOException if an IOException occurs during the exec of the
>>>> policytool process
>>>> +     */
>>>> +    private static void policyToolLaunchHelper(final JFrame frame, final
>>>> String filePath) {
>>>> +        final String[] launchCommand = new String[] { "policytool", "-file",
>>>> filePath };
>>>> +        new Thread(new Runnable() {
>>>> +            @Override
>>>> +            public void run() {
>>>> +                try {
>>>> +                    final Process ptool =
>>>> Runtime.getRuntime().exec(launchCommand);
>>>> +                    if (ptool.waitFor() != 0) {
>>
>> You do realize that Process.waitFor() may never return and thus block the
>> current thread indefinitely? So, either some other thread has to interrupt it
>> and kill that sub process (or better yet, this thread's InterruptedException
>> handler). Or, the sub process needs to be polled by Process.exitValue() after
>> some timeout and then perhaps be Process.destroy()'ed.
>
> Calling Process#exitValue() on a process which has not yet terminated is
> illegal, and I really don't think having some timeout to destroy the process is
> a good idea.

Just because Process.exitValue() may throw an IllegalThreadStateException at run 
time, it does not mean it is illegal to call it on any not yet terminated 
process. It is legal to call it anytime indeed. Obviously, the designers of 
Process have chosen to return the process's state data via an exception, simply 
because returning state data through the return value is not feasible. If both 
kinds of data, the process's state and its return value, were returned through 
Process.exitValue()'s return value, this data would be indistinguishable.

What I actually have meant was that the process should be checked whether it had 
started yet. If not, Process.destroy() it. But, to be honest, I have not figured 
out how to do that either.

> There is no way to do this and distinguish between the policytool
> process erroneously continuing to run (eg perhaps it has become a zombie) and
> the policytool still being in use by the user.

Yeah, being able to detect a whether a process has gone zombie after 
Process.exec() or ProcessBuilder.start() would be helpful indeed.

> But as you say, adding a destroy() call in an InterruptedException handler is a
> good idea. If the helper thread is interrupted then the process it spawned
> should be cleaned up. The problem is that even if we add this handler, when will
> the helper ever have interrupt() called on it? If we're really worried about
> ensuring that the policytool process is definitely cleaned up when we exit the
> control panel (so that the control panel can cleanly exit as well), then we need
> to add some logic to ensure Thread#interrupt() is called when the window is
> closing, correct? However, we still don't have a way to know if the policytool
> process is running simply because the user hasn't finished using the policytool
> yet, or because it has become a zombie - and adding a listener to call
> interrupt() on the helper thread means that closing the control panel will quit
> the policytool, probably without even asking if you want to save your changes
> first. Is this really what we want?
>
>>
>>>> + reflectivePolicyToolLaunch(filePath);
>>>> +                    }
>>>> +                } catch (Exception e) {
>>
>> I don't see any InterruptedException handling here, do I? Well, that's one
>> reason why you should not "willy nilly" catch all exceptions into one handler.
>> In case of an InterruptedException Process.destroy() should be called here.
>> (head shake) :-(
>
> Yes, this is a good point in general. Thanks for pointing it out. But as above,
> I think if we're going to bother with trying to ensure cleanup for corner cases,
> then we need to properly define when cleanup happens first. A separate
> InterruptedException will get put in, don't get me wrong, but there's more to
> discuss first I think.
>
>>
>>>> + OutputController.getLogger().log(e);
>>>> +                    showCouldNotOpenFileDialog(frame, filePath,
>>>> R("CPPolicyEditorNotFound"));
>>>> +                }
>>>> +            }
>>>> +        }).start();
>>>> +    }
>>>> +

Jacob