RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v7]

[Mandy Chung]

On Fri, 11 Feb 2022 13:51:38 GMT, liach <duke at openjdk.java.net> wrote:

>> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), by design, duplicate initialization of ReflectionFactory should be safe as it performs side-effect-free property read actions, and the suggesting of making the `initted` field volatile cannot prevent concurrent initialization either; however, having `initted == true` published without the other fields' values is a possibility, which this patch addresses.
>> 
>> This simulates what's done in `CallSite`'s constructor for `ConstantCallSite`. Please feel free to point out the problems with this patch, as I am relatively inexperienced in this field of fences and there are relatively less available documents. (Thanks to https://shipilev.net/blog/2014/on-the-fence-with-dependencies/)
>
> liach has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - The fast path should always come first. good lesson learned!
>    restore config field comments
>  - Try making the config a record and see if it works

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 620:

> 618:      * The configurations exist as an object to avoid race conditions.
> 619:      * See bug 8261407. The object methods backed by indy may not be available.
> 620:      */

This comment is a bit unclear.   With the suggestion of moving the static `ReflectionFactory::config()` method and the comment I suggest below, I think that should be adequate to understand.  If not, we should improve the comment rather than relying on the readers to read the bug report.

Maybe the comment can be:

Configuration for core reflection which is configurable via system properties.
The user-configured settings are not available during early VM startup.

Note that the object methods of this Config record are called via indy which is 
available to use after initPhase1.   We can workaround that limitation by
implementing the object methods.

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 631:

> 629:             // To avoid this penalty we reuse the existing JVM entry points
> 630:             // for the first few invocations of Methods and Constructors and
> 631:             // then switch to the bytecode-based implementations.

This block of comment describes why the default for `noInflation` is false.   This can be moved to L645 as the comment for the `DEFAULT` config.

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 638:

> 636:             // true if deserialization constructor checking is disabled
> 637:             boolean disableSerialConstructorChecks
> 638:     ) {

Formatting: preferable to follow the style of the local file, e.g. like the `newConstructor` method declaration


private record Config(boolean noInflation,
                      int inflationThreshold,
                      int useDirectMethodHandle,
                      boolean useNativeAccessorOnly,
                      boolean disableSerialConstructorChecks) {
```            

Same comment for L645-651 and L719-725

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 660:

> 658:          * run, before the system properties are set up.
> 659:          */
> 660:         private static @Stable Config instance;

The comment makes me think that this static field and the static `instance` method belong to `ReflectionFactory` class.  The static initializer of java.lang.reflect.Method/AccessibleObject causes this class (ReflectionFactory) to be initialzed early during VM initialization where the configuration is not available.   The `Config` class is just a data type representing the setting.  When the `Config` class is initialized, it's irrelevant.  That should help the readers easier to understand.

A minor update to the comment may help too:

/**
 * The configuration is lazily initialized after the module system is initialized.
 * 
 * The static initializer of this class is run before the system properties are set up.
 * The class initialization is caused by the class initialization of java.lang.reflect.Method
 * (more properly, caused by the class initialization for java.lang.reflect.AccessibleObject)
 * that happens very early VM startup, initPhase1.
 */
 private static @Stable Config config;


If this static field is moved back to ReflectionFactory class, your original variable name `config` and the accessor method `config()` is clear.   `Config::load` can be renamed to `Config::instance` then.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6889