RFR 7149320: Move sun.misc.VM.booted() to end of System.initializeSystemClass()

[David Holmes]

Okay - Alan has pointed out that fairly recently (6888546) 
setJavaLangAccess and booted() were in the opposite order to today. 
Based on that I now think it is extremely low risk to change the order 
as suggested.

Thanks,
David

On 29/02/2012 11:15 AM, David Holmes wrote:
> Hi Mike,
>
> On 29/02/2012 5:23 AM, Mike Duigou wrote:
>>
>> On Feb 27 2012, at 20:50 , David Holmes wrote:
>>
>>> Hi Mike,
>>>
>>> The problem with changing initialization order is that you can never
>>> enumerate all the possible initialization paths. I don't see anything
>>> problematic with the move in relation to Thread.currentThread() or
>>> ThreadGroup.add(). But the call to setJavaLangAccess requires
>>> initialization of SharedSecrets which in turn requires initialization
>>> of Unsafe which in turn initializes sun.reflect.Reflection which in
>>> turn initializes a bunch of Collection classes - and suddenly we may
>>> have a whole swag of classes now being initialized before the VM
>>> appears to be booted.
>>
>> The class initialization I'm interested in does occur during the
>> SharedSecrets initialization path. Because, in the current
>> implementation, booted() hasn't been called the only way I can
>> determine if the class is being used before boot() is to check if the
>> SharedSecrets have been initialized. This ends up being cumbersome and
>> permanently leaves some checking logic in the execution path for what
>> is a very narrow window. I've looked at where isBooted is currently
>> being used and none of the uses would seem to be sensitive to being
>> called
>
> Could Win32ErrorMode.initialize() now be called too late in some case?
> What about sun.nio.cs.ext.ExtendedCharsets.init()?
> ...
>
>>> If you then throw into the mix the possibility of different system
>>> properties affecting initialization actions, or the existence of an
>>> agent, then who knows what might actually happen.
>>
>> Do you believe that the change is as a result too risky to consider?
>> Are there validations that could/should be done to reduce or eliminate
>> the risk?
>
> I think this change may well have unanticipated consequences that might
> take a long time to surface. If I was confident that regular testing
> might expose these then I'd say go ahead, but I think these will be
> obscure changes in very specific contexts that probably are not tested.
> Given it is only a convenience fix is it worth any risk? You could
> always add a new field to reflect the end of initializeSystemClass. Or
> make booted an integer:
>
> private static volatile int booted = 0;
> public static void booted() {
> booted++;
> }
> public static boolean isBooted() {
> return booted > 0;
> }
> public static void fullyBooted() {
> booted++;
> }
> public static boolean isFullyBooted() {
> return booted > 1;
> }
>
> Cheers,
> David
>
>
>>
>>> Cheers,
>>> David
>>> -----
>>>
>>>
>>> On 28/02/2012 1:57 PM, Mike Duigou wrote:
>>>> Hello all;
>>>>
>>>> This issue is a patch for review that I have split out of a larger
>>>> issue I'll be submitting later.
>>>>
>>>> WEBREV @ http://cr.openjdk.java.net/~mduigou/7149320/0/webrev/
>>>>
>>>> sun.misc.VM.booted() is currently called before setJavaLangAccess().
>>>> For code which uses the JavaLangAccess shared secrets it's
>>>> convenient to be able to check whether the secrets are initialized
>>>> without having to call sun.misc.SharedSecrets.getJavaLangAccess()
>>>> every time the secrets are used and checking for null. In particular
>>>> with the static inner class holder idiom it would be desirable to do :
>>>>
>>>> static class Holder {
>>>> final sun.misc.JavaLangAccess ACCESS =
>>>> sun.misc.SharedSecrets.getJavaLangAccess();
>>>> }
>>>>
>>>> ...
>>>>
>>>> if(sun.misc.VM.isBooted()&& Holder.ACCESS...
>>>>
>>>> In my research on this issue I was unable to determine why
>>>> sun.misc.VM.booted() isn't the currently the last activity in
>>>> System.initSystemClass(). Neither of the two tasks which currently
>>>> follow it depend upon booted state in any way that I could
>>>> determine. I am tempted, thinking about it, to add a comment to
>>>> System.initSystemClass before the call to sun.misc.VM.booted()
>>>> asking future maintainers to leave boot() as the last activity or
>>>> explain why not.
>>>>
>>>> I have done JTReg runs on linux x86, linux x64 and Solaris 64 which
>>>> incorporated this change without any problems encountered. It's
>>>> rather difficult to write a unit test for this issue as it requires
>>>> a class that is initialized after java.lang.System but before
>>>> java.lang.System.initSystemClass() and uses JavaLangAccess.
>>>>
>>>> Thanks,
>>>>
>>>> Mike
>>