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

[Jacob Wisor]

On 01/22/2014 07:57 PM, Andrew Azores wrote:
> On 01/22/2014 07:26 AM, Jacob Wisor wrote:
>> On 01/22/2014 01:30 AM, Jacob Wisor 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)
>>>>>>
>>>>>> 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.
>>> [...]
>
> 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?

First of all, the term "scope" in the context of programming languages only 
refers to visibility of memory resource identifiers from a given section of 
source code. It has no implications on nor any assumptions can be made based 
upon it about the visibility of memory resources in any target execution 
environment. A given programming language scope may or may not have an 
equivalent abstraction, representation, or constraint in the target execution 
environment. In Java, both cases can be found, depending on the language 
construct. So, please do not confuse applying the term "scope" from a 
programming language to an execution environment. Most execution environments do 
not deal with scopes. They rather have limitations on addressing modes or have 
memory management units etc.

In the source code at hand, Runnable.run() does not lose scope of launchCommand. 
Otherwise, the source code would not compile, simply because scope checking is 
done at compile-time, never at run-time. However, what bothered me was that 
Runnable.run() needs to access the object via the reference stored in the local 
constant variable launchCommand. Accesses can only be accomplished through 
reading the value of a field, an array element, or a local variable (or a return 
value that is also placed on the stack). And, since launchCommand isn't passed 
and stored nowhere in the source code explicitly to Runnable, I would rather 
assume it is not going to be available at run-time. So, I decided to dig deeper 
into this by disassembling the relevant classes. As it turns out, the OpenJDK 7 
compiler synthesizes a "final synthetic java.lang.String[] val$launchCommand" 
field in Runnable. IMHO this is a violation of the Java Language Spec. It is no 
violation of the JVM Spec, of course. But, it is annoying and confusing that the 
so called "Java reference implementation" silently starts to violate the Java 
Language Spec in order to get around corner cases or to patch things up so that 
it just runs. :-( This means that you can never be sure about the code that the 
compiler is going to generate or rather you have to be always aware that it will 
possibly generate some wired and dubious stuff into your byte code. This is 
totally against Java's original intention of a compile-time error preventing 
type-safe verifiable platform. *rant start* They have turned this idea upside 
down by eliminating errors silently or implicitly instead of leaving programmers 
in control of their programs and the way they want to solve problems. Why did 
they do that? Did they fear some programmers being to dump to solve problems 
themselves? *end of rant*

> This to me seems to imply that closures are utterly broken in Java, if the enclosed context
> may suddenly become unavailable altogether.

Relax, they are not broken. ;-) And, even if the compiler would not synthesize 
fields into Java classes closures would still work. The only difference would be 
that the compiler would issue an error when thread-crossing accesses to 
enclosing local variables from enclosed classes appear in the source code.

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

Well, this is actually what the OpenJDK 7 compiler does for you silently anyway. 
Or, maybe automagically! :-D

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

Ah no, the code would not be more or less difficult to maintain than any other 
threading related code.

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

So, I guess you can leave it the way it is, unless you want to make the source 
code imho truly Java Language Spec conforming so that other truly conforming 
Java compiler implementations won't error out on this construct. ;-)

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

I did not say nor imply that OpenJDK or any other JVM implementation would not 
be properly implemented. I just would have expected, according to the Java 
Language Spec that the JVM would throw some kind of RuntimeException or Error at 
run-time at this point.

> 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[], it is 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.

The life cycle of the object was never of concern to me. The variable holding 
the reference was. And, as it turns out, the reason for declaring variables 
final for use in closures is actually for the compiler to do stuff behind your 
back. :-\

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

Right, with ProcessBuilder at hand catching InterruptedException becomes 
obsolete in this case. Yet, I would still go with instantiating and operating on 
ProcessBuilder on a separate thread because ProcessBuilder.start() probably is 
or may be on some systems synchronous. Actually, there is no way to know for how 
long ProcessBuilder.start() will block the current thread, so it always seems 
better to avoid blocking the AWT thread as much as possible.

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

Same as above, I guess you can depend on the compiler implicitly synthesizing 
any necessary fields into the anonymous class.

Jacob