Open charpov opened 1 year ago
I'm not familiar with the code, but I think you are correct.
I would expect final fields holding both ctor args. Then it's OK to use the mutable cache field to produce the object in the longEncoding
case ("possibly wastefully").
Looking at the discussion history on the PRs, I see there is an intersection of considerations that make the scheme complex, so I think you're right that the reviewer had other issues besides safe construction.
Edit: OTOH, I couldn't produce a bad read using simple threads reading/writing unsafely.
Edit: OTOH, I couldn't produce a bad read using simple threads reading/writing unsafely.
I tried too, but it's going to be hard, if at all possible. You need a scenario in which you write _bigInteger
with an object, then longValue
, which trigger what the JLS calls a "freeze" because longValue
is final
, then still read _bigInteger
as null
from another thread that invokes bigInteger()
(that's the only place; all other accesses to _bigInteger
are safe). Depending on how the "freeze" is implemented, it might actually create more of a memory barrier than what the JLS requires, thus making the scenario impossible on this particular implementation. We can agree that this is not something to lose our sleep over (especially since bigInteger
is not used within the code and probably not much outside), but if adding val _long = this._long
at the beginning of bigInteger()
solves the theoretical problem, it might still be worth adding (it cannot possibly hurt).
Sounds right to me too, thanks!
I believe that
BigInt
, which is nominally immutable, is not strictly thread-safe because of the non-final field_bigInteger
. IfN
is a JavaBigInteger
, there is not enough happens-before between a thread doingx = new BigInt(N)
and another doingx.bigInteger
, which could return aBigInteger
equal toLong.MinValue
instead ofN
.bigInteger
is implemented as:The comment about atomicity ignores memory visibility issues, which are not discussed anywhere in the source file. In the scenario above,
read = _bigInteger
could readnull
instead ofN
and create an incorrectBigInteger
from_long
(which isLong.MinValue
in this case).One solution is to add a memory fence at the end of the
BigInt
constructor. Alternatively, since_long
is final and initialized after_bigInteger
(assuming fields are initialized in declaration order), one could always read_long
before reading_bigInteger
. This seems to be the case everywhere in the source, except inside thebigInteger
method. So, addingval _long = this._long
at the beginning of this method (beforeval read = ...
) should be sufficient.Note that there is no danger of
_bigInteger
being neitherN
nornull
becauseBigInteger
is immutable (except for a few lazily initializedint
fields which are not an issue).Maybe I'm missing something subtle and the code is correct, but because the comments in the source of
BigInt
never mention JMM issues, I wonder if they have been considered at all.