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

[Andrew Azores]

On 01/23/2014 01:54 PM, Jacob Wisor wrote:
> On 01/22/2014 07:57 PM, Andrew Azores wrote:
>> 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.

You're right that my use of the word "scope" previously was an abuse of 
the term. Yes, I did use it when talking about a runtime execution 
environment even though it's generally only a term used at the source 
level. I think it should be pretty obvious that I know that using an out 
of scope variable means non-compiling code. Forgive me for trying to use 
simplified language to get a point across without writing a technical 
essay of every bare metal implementation detail involved.

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

I *absolutely*, fundamentally disagree with your assertion that giving 
programmers the language construct of closures means that they have made 
the language impossible for you to reason about and that they have 
removed your power to solve a problem the way you want. Quite the 
opposite. If you don't like that the compiler can make your anonymous 
inner classes retain implicit references to their context, then feel 
free to not make use of the notion of closures in your code. Create 
named inner classes with fields to hold references instead and the 
compiler will still accept it. However, I know about closures, I know 
that Java has them, and I feel that in some cases they make my code more 
concise, and so I am very glad that the language does not force me to 
code in the style that you prefer. I also highly disagree with your idea 
that implementing closures is equivalent to silently handling errors, 
because there is nothing erroneous about the behaviour. They've been in 
Scheme for nearly 40 years now and were defined for a decade before 
that, and pretty much any halfway decent functional language is going to 
have full support for closures. This is a rich history of a well-defined 
conceptual language feature, so I cannot agree that anyone is at fault 
for including it in Java.

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

So you would only have single-threaded closures, then. I would still 
call that "utterly broken in Java" - this means that closures would be 
relying on being used in a specific context (single-threaded 
programming) that they really should be independent of, especially in a 
language that has a good model and support for multi-threading. And, as 
you've discovered (the hard way ;-) ), the solution is really not very 
complex. Considering that the "anonymous Runnable inside a 
Thread(Runnable) constructor" pattern is so prevalent, I think it's a 
worthy thing to ensure is done right. Not only is the solution in use 
here the seemingly most common design pattern taken by other languages 
that implement closures, it also seems like a very simple way to ensure 
that you do it correctly even if you do want to rely on single threaded 
execution. It's really not much more complicated than making anonymous 
classes just use the same references as present in their enclosing context.

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

Yes, exactly. Why should I do work when I know a trick to make the 
compiler do it for me, *and* I know exactly how the compiler achieves it?

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

My point about maintainability is that I think introducing a brand new 
named class, even an inner one, is not necessary when the class will 
simply be a Runnable with one or two fields. This is going to make the 
enclosing class much more cluttered than it has to be and for no good 
reason.

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

I appreciate your concern about portability but I am not willing to 
change this implementation for fear of being incompatible on systems 
where the compiler or JVM does not support anonymous inner classes being 
closures. That's a core language feature and I'm not going to code 
around it in fear of breaking compatibility with half-baked Java 
environments.

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

Well, you did essentially say that you thought that multithreading 
breaks closures, which to me would mean that something has been 
implemented badly. I don't think the quote you gave before from the 
Language Spec is violated by the implementation, at all. It is still 
certainly true that the two Threads are not sharing the same local 
variables or anything. They are sharing a heap object, but they each 
have their own distinct reference variable with which to refer to it. 
Yes, it's true that one of these references is not explicitly declared, 
but it is still present, and everything works as it should since this 
"implicitly declared" reference is made as a copy of the explicitly 
declared reference. And this is why the explicitly declared one must be 
final - so that the original and the copy reference are both guaranteed 
to always refer to the same heap object.

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

Well I wouldn't call it "behind your back" since the behaviour is meant 
to be well-known, but yes. This right here is the entire source of the 
miscommunication we had - you weren't aware of the significance of using 
final variables for closures, but I was :-) and I wasn't sure exactly 
what about the model I knew that you didn't, so my explanation ended up 
including some stuff that I was /pretty/ sure you knew, but didn't want 
to take for granted.

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

That was my whole point. This is not "behind your back" or a violation 
of the language spec - this is just what a closure is in general 
programming terms, and Java anonymous inner classes implement closures 
nicely with a multithreading compatible model.

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

Anyway, moving on to the actual work at hand :-). Again, you're right 
that ProcessBuilder#start() doesn't seem to be explicitly defined as 
being asynchronous, although intuitively it seems like it should be. But 
I can't find hard facts anywhere guaranteeing this, and I can't really 
see it causing any problems to do this work within a new Thread (because 
what possible issue could there be with removing this work from the AWT 
thread? This absolutely should be independent of that). So in the new 
patch, that whole chunk of code is inside a new Thread.

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: custompolicy-exec-with-fallback2.patch
Type: text/x-patch
Size: 13678 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140123/02a05c2a/custompolicy-exec-with-fallback2-0001.patch