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

[Mandy Chung]

On Thu, 10 Feb 2022 02:24:43 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
> 
>  - use peter's model of separate data object
>  - Merge branch 'master' into 8261407-reflectionfactory
>  - Include the stable annotation
>  - Merge branch 'master' into 8261407-reflectionfactory
>  - Merge branch '8261407-reflectionfactory'
>  - Just use volatile directly to ensure read order
>  - 8261407: ReflectionFactory.checkInitted() is not thread-safe

This looks good.   I have a couple of small suggestions.

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

> 619:         java.lang.reflect.AccessibleObject) causes this class's to be
> 620:         run, before the system properties are set up. */
> 621:     private static Config config;

This can be made `@Stable` as this is initialized only once.

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

> 641:      */
> 642:     private static final class Config {
> 643:         private static final Config fallback = new Config(false);

Suggestion:

        private static final Config DEFAULT = new Config(false);


I suggest to rename `fallback` to `DEFAULT`.

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

> 670:         private final boolean disableSerialConstructorChecks;
> 671: 
> 672:         private Config(boolean getProperties) {

I suggest to make this a static `Config.getInstance()` method to return a `Config` instance and that should assert if this is called after the module system is initialized.   The `Config` constructor is a simple constructor taking one parameter for each field and just set all fields to the parameter value.

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

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