RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v4]
Mandy Chung
mchung at openjdk.java.net
Wed Feb 9 20:36:17 UTC 2022
On Sat, 22 Jan 2022 00:05:49 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 one additional commit since the last revision:
>
> Include the stable annotation
> [plevart at b62c2ce](https://github.com/plevart/jdk/commit/b62c2ce9673e40b5b051ba5962c6bfa34e172a87)
> ...using this approach, the initialization is guaranteed to happen when any of the fields in Config class needs to be accessed from wherever it may be without making sure that checkInitted() is called at appropriate time. This is, IMHO, more robust than current approach and less prone to bugs. And you get guaranteed visibility of final fields in Config as a bonus without relying on ordering of writes/reads with volatile boolean initted..
I like this approach, more reliable and cleaner. Nit: instead of calling `config().noInflation`, it can just call the static `noInflation()` method. Similar for other fields.
-------------
PR: https://git.openjdk.java.net/jdk/pull/6889
More information about the core-libs-dev
mailing list