BigInt not strictly thread-safe

[charpov]

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:

  def bigInteger: BigInteger = {
    val read = _bigInteger
    if (read ne null) read else {
      val write = 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.

[som-snytt]

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.

[charpov]

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).

[lrytz]

Sounds right to me too, thanks!