"Java concurrency in practice" - cached thread-safe number factorizer (Listing 2.8)

user_493928 0
Viewed 505

In the following code (copied from Java Concurrency in Practice Chapter 2, section 2.5, Listing 2.8):

@ThreadSafe
public class CachedFactorizer implements Servlet {
    @GuardedBy("this") private BigInteger lastNumber;
    @GuardedBy("this") private BigInteger[] lastFactors;
    @GuardedBy("this") private long hits;
    @GuardedBy("this") private long cacheHits;

    public synchronized long getHits() { return hits; }

    public synchronized double getCacheHitRatio() {
        return (double) cacheHits / (double) hits;
    }

    public void service(ServletRequest req, ServletResponse resp) {
        BigInteger i = extractFromRequest(req);
        BigInteger[] factors = null;
        synchronized (this) {
            ++hits;
            if (i.equals(lastNumber)) {
                ++cacheHits;
                factors = lastFactors.clone(); // questionable line here
            }
        }
        if (factors == null) {
            factors = factor(i);
            synchronized (this) {
                lastNumber = i;
                lastFactors = factors.clone(); // and here
            }
        }
        encodeIntoResponse(resp, factors);
    }
}

why the factors, lastFactors arrays are cloned? Can't it be simply written as factors = lastFactors; and lastFactors = factors;? Just because the factors is a local variable and it is then passed to encodeIntoResponse, which can modify it?

Hope the question is clear. Thanks.

local-variables thread-safety clone java
5 Answers

I agree that that section of the book could have been better explained by the author of the book.

It is true that, in order to implement thread safety properly, you have to synchronize both read and write operations using the same lock; in the code above, in order to minimize the amount of synchronization, the author decided to perform the encodeIntoResponse(...) without synchronization: since the encodeIntoResponse(...) method reads the content of the array referenced by factors then the author cloned it into a new array.

Note: while it is true that factors is a local variable, in general, it would still require to be cloned because the same array is read by synchronized and not synchronized code, which could happen if we pass the reference (without cloning) to lastFactors and encodeIntoResponse(...).

However, as correctly pointed out by @khachik in the question and by @david-harkness in a response, in this specific case the clone calls are unnecessary because lastFactors is safely published, and it is not modified after its publication.

user_69365550

If you change factors = lastFactors.clone(); to factors = lastFactors;, both factors and lastFactors point to the same object, factors is no longer a local variable, it becomes a shared mutable state.

Imagine there are three requests, request A, B, C. The number sent by Request A and B is 10, but the number sent by request C is 20. Things can go wrong if the below execution order happens and you change factors = lastFactors.clone(); to factors = lastFactors;.

  1. servlet server receives request A, the entire service method is executed, now lastNumber is 10, lastFactors is [1, 2, 5, 10].
  2. servlet server receives both request B and C, request B is handled at first, but after exiting the first synchronized block (now for request B, factors is [1, 2, 5, 10], which is correct), request C is handled.
  3. for request C, the entire service method is executed, it change lastFactors from [1, 2, 5, 10] to [1, 2, 4, 5, 10, 20], because both factors lastFactors point to the same object, factors is now [1, 2, 4, 5, 10, 20] too. The response of request B is supposed to be [1, 2, 5, 10], but is [1, 2, 4, 5, 10, 20] right now.
user_52322550
Related