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

[Andrew Azores]

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

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 "milly 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

Thanks,

-- 
Andrew A