You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I believe that BigInt, which is nominally immutable, is not strictly thread-safe because of the non-final field _bigInteger. If N is a Java BigInteger, there is not enough happens-before between a thread doing x = new BigInt(N) and another doing x.bigInteger, which could return a BigInteger equal to Long.MinValue instead of N.
bigInteger is implemented as:
defbigInteger:BigInteger= {
valread= _bigInteger
if (read ne null) read else {
valwrite=BigInteger.valueOf(_long)
_bigInteger = write // reference assignment is atomic; this is multi-thread safe (if possibly wasteful)
write
}
}
The comment about atomicity ignores memory visibility issues, which are not discussed anywhere in the source file. In the scenario above, read = _bigInteger could read null instead of N and create an incorrect BigInteger from _long (which is Long.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 the bigInteger method. So, adding val _long = this._long at the beginning of this method (before val read = ...) should be sufficient.
Note that there is no danger of _bigInteger being neither N nor null because BigInteger is immutable (except for a few lazily initialized int 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.
The text was updated successfully, but these errors were encountered:
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).
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.The text was updated successfully, but these errors were encountered: