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

[Andrew Azores]

On 01/22/2014 07:26 AM, Jacob Wisor wrote:
> W dniu 22.01.2014 01:30, Jacob Wisor pisze:
>> 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)
>>>>>
>>>>> Ouch. See my reply to patch.
>>>>
>>>> Didn't see any mail, but we discussed on IRC. Here's a recap:
>>>> - The reflection hack is horrible. It doesn't quite work, and it's 
>>>> super ugly.
>>>> - The process not exiting is definitely a problem. We should not be 
>>>> playing
>>>> around with reflection on the policytool like this.
>>>> - Calling PolicyTool.main() directly breaks the automated build(?). 
>>>> We don't
>>>> want to make it a build dep, and it's too minor to bother with a 
>>>> configure
>>>> step/flag(?).
>>>> - Solution: Runtime#exec() makes its return, with a fallback method of
>>>> reflectively invoking PolicyTool.main() if for any reason the exec 
>>>> fails
>>>> (non-POSIX system, executable not on PATH, etc)
>>>>
>>>> So here's a patch implementing this solution, with lots of extra added
>>>> documentation.
>>>>
>>>
>>> And this time with better respect for event dispatch thread, I hope. 
>>> (thanks
>>> omajid)
>>
>> +    private static void policyToolLaunchHelper(final JFrame frame, 
>> final String
>> filePath) {
>> +        final String[] launchCommand = new String[] { "policytool", 
>> "-file",
>> filePath };
>>
>> Ah, the caveats of anonymous classes and threads again: The 
>> launchCommand local
>> constant is not guaranteed to be available when Runnable.run() is 
>> scheduled to run.
>>
>> +        new Thread(new Runnable() {
>> +            @Override
>> +            public void run() {
>> +                try {
>> +                    final Process ptool =
>> Runtime.getRuntime().exec(launchCommand);
>>
>> The launchCommand local constant referenced here which lives on the 
>> stack of the
>> anonymous class's enclosing policyToolLaunchHelper() method is not 
>> guaranteed to
>> be valid or to exist after the thread had been started. 
>> policyToolLaunchHelper()
>> may terminate and thus its local stack be cleaned up /before/ 
>> Runnable.run() is
>> scheduled to run, leaving no constant for reference. I am not 
>> entirely sure
>> about what the Java Programming Language Spec and/or JVM Spec define 
>> in this
>> case but I would bet that it has intentionally been left undefined. 
>> There is
>> certainly no problem when anonymous class's methods and their 
>> enclosing methods
>> share local variables running on the same thread. This case has been 
>> defined.
>> But, in the case of threads, I would be rather careful. Sorry, I do 
>> not have the
>> time to look it up in the specs. :-/
>
> Well, I did look it up because it was quite an intriguing question. 
> Sub-paragraph 3 of Java Language Specification §17.4.1 says it:
> "Local variables (§14.4), formal method parameters (§8.4.1), and 
> exception handler parameters (§14.20) are never shared between threads 
> and are unaffected by the memory model." ;-)
>
>> To get thread-safe, you would have to pass launchCommand as a 
>> parameter to
>> Runnable's constructor and store its reference in a member variable.
>>
>> Btw, the preferred way to construct Process objects since JDK 1.5 is 
>> to use
>> ProcessBuilder. ProcessBuilder would also allow you to set 
>> policytool's current
>> working directory to user.home, which is advisable.
>> [...]
>
> Jacob

Amazing, thank you for all the help with this! But are you sure about 
this problem of the constant going out of scope of the Thread? Yes, 
variables, method parameters, and exception handler parameters will 
never be *shared* between threads (as in each Thread will have a local 
variable by the same name but these two will be distinct from each other 
in memory), but does this really mean that the variable can go out of 
scope for the Thread by the time it runs? This to me seems to imply that 
closures are utterly broken in Java, if the enclosed context may 
suddenly become unavailable altogether. And if this is true, that means 
that no matter what we do when we're spawning a new Thread, we always 
have to make a new class for it that implements Runnable and has fields 
to copy absolutely any state data that we need to copy from our context 
(to avoid relying on closures). This is going to end up looking 
horrendous and be very difficult to maintain, while being used to 
implement what should be a very simple and highly non-critical piece of 
the software. Assuming that the launchCommand variable becomes 
unavailable by the time the new Thread is going to run, the end result 
is just the the policytool fails to launch due to an NPE or something. 
As long as this isn't the guaranteed behaviour, or the most likely 
behaviour, then maybe it's a rare edge case that we can simply live with?

But I really am not convinced that this is a problem we need to worry 
about anyway. I am pretty sure that closures are well implemented and 
that this problem should never occur in any properly functioning JVM 
implementation. This is the entire reason that things need to be 
declared final before you can use them inside of a closure - so that a 
copy of the reference can be made for the anonymous class to hold on to, 
and that reference is guaranteed to never change while the anonymous 
class has it.And, since the constant in question here is a String[], 
itis not a primitive and therefore although the first reference to it is 
a stack variable, the object itself lives on the heap. Then the 
anonymous function/closure's reference to it is a second strong 
reference to the same object. So even if the method exits and its stack 
frame disappears along with the local reference to it, there is still 
SOME strong reference to the memory object, namely within the closure. 
Thus it is reachable and will not be GC'd until after the Thread 
completes and discards its Runnable, destroying the closure's context 
and removing the object reference, and in this case, the last reference 
- freeing the object's memory allocation to be GC'd on the next cycle.

Now, if we're going to use ProcessBuilder instead (which I think is a 
good idea, thanks for this recommendation as well), do we really even 
need to be constructing a separate Runnable to feed into a new Thread? 
ProcessBuilder.start() will throw an IOException if the process can't be 
started, eg if the policytool executable isn't available, or what have 
you. So we just catch this IOException and use the fallback launch here. 
If ProcessBuilder.start() doesn't throw an exception then we may assume 
that the policytool window has successfully opened and as far as we're 
concerned, the launch is complete. We don't need to launch a new thread 
to asynchronously monitor the process using waitFor(), because we don't 
really need to check for a non-zero exit value - the potentially thrown 
IOException is doing this job for us now. As far as I can tell we also 
don't need to worry about doing any Process#destroy() anymore either, 
because if ProcessBuilder#start() throws an IOException then we never 
got a Process to begin with, and if it doesn't throw then the assumption 
is that, well, everything is okay and no destroy() is needed. There's 
also no InterruptedException to worry about dealing with either because 
we don't have any Threads doing any kind of waiting.

What about the fallback reflective launch method, though? I suppose we 
still want to spawn a new Thread to handle this? If we do (which I 
suspect is the case), then are we still truly concerned about the 
filePath String being unavailable to the Thread/its anonymous Runnable? 
This means we need to make a Runnable implementation that holds 
references to not only the filePath String, but also to the relevant 
data used to display an error dialog (JFrame, message String). This 
sounds, honestly, ludicrous to me... as above, I am *pretty* sure that 
we can depend on closures being well implemented.


> p.s.: Oh, one more thing: Please post rfc patches always based on tip, 
> not based on previously posted rfc patches. Otherwise they are 
> difficult to track. Thank you. 

Hmm? I haven't posted any patches based off other patches - they've all 
been off of tip. Maybe it got confusing because at one point one of the 
older patches in this thread actually did get OK'd and pushed, and 
immediately after more issues were discovered, so we are back to 
revising. So these latest patches actually are based off tip, 
unfortunately :(

Thanks,

-- 
Andrew A

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140122/370e15c3/attachment.html