Correctly synchronizing/locking access to an ArrayList

147
August 12, 2021, at 4:20 PM

I am working on my first Spring-Boot application, after not touching Java in ages. Below is one of my classes.

import org.apache.commons.csv.CSVFormat;
import org.apache.commons.csv.CSVRecord;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;
import org.springframework.scheduling.annotation.Scheduled;
import javax.annotation.PostConstruct;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.Reader;
import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;
import java.text.SimpleDateFormat;
import java.util.Date;
@Component
public class InventoryService {
    private LocationService locationService;
    private class entry
    {
        public String productId;
        public String location;
        public int    quantity;
    };
    private ArrayList<entry> storeInventory;
    private ArrayList<entry> onlineInventory;
    private final int THRESHOLD = 2;
    private final int RATE = 15000;
    @Autowired
    public InventoryService(LocationService locationService) {
        this.locationService = locationService;
        storeInventory = new ArrayList<entry>();
        onlineInventory = new ArrayList<entry>();
    }
    @PostConstruct
    public synchronized void readInInventory() throws IOException {
        try(Reader in = new InputStreamReader(getClass().getResourceAsStream("/product_inventory.csv"))) {
            Iterable<CSVRecord> records = CSVFormat.DEFAULT
                    .withHeader("productId", "location", "quantity")
                    .withFirstRecordAsHeader()
                    .parse(in);
            onlineInventory.clear();
            storeInventory.clear();
            for (CSVRecord record : records) {
                String productId = record.get("productId");
                String location = record.get("location");
                int quantity = Integer.parseInt(record.get("quantity"));
                entry e = new entry();
                e.productId = productId;
                e.location = location;
                e.quantity = quantity;
                if (location.equals("ONLINE")) {
                    onlineInventory.add(e);
                } else {
                    storeInventory.add(e);
                }
                System.out.println(productId + "\t" + location + "\t" + quantity);
            }
        }
    }
    private static final SimpleDateFormat dateFormat = new SimpleDateFormat("HH:mm:ss");
    @Scheduled(fixedRate = RATE)
    public void updateInventory() {
        System.out.println("The time is now " + dateFormat.format(new Date()));
        try {
            readInInventory();
        }
        catch (Exception e) {
            System.out.println("Exception: " + e.toString());
        }
    }
    public boolean hasInventoryOnline(Product product, int quantity) {
        String productId = product.getProductId();
        entry found = onlineInventory.stream()
            .filter(e -> productId.equals(e.productId))
            .findFirst()
            .orElse(null);
        if (found == null) {
            return false;
        }
        // if I understood the task correctly, online we allow 1 item
        return found.quantity > 0;
    }
    public boolean hasInventoryInNearbyStores(Product product, int quantity, Location currentLocation) {
        String productId = product.getProductId();
        List<Store> nearby = locationService.getNearbyStores(currentLocation);
        List<entry> nearbyStoreInventory = storeInventory.stream()
            .filter(inv -> nearby.stream()
                .anyMatch(s ->
                    inv.location.equals(s.getStoreName())))
                .collect(Collectors.toList());
        entry found = nearbyStoreInventory.stream()
            .filter(e -> productId.equals(e.productId))
            .findFirst()
            .orElse(null);
        if (found == null) {
            return false;
        }
        return found.quantity > THRESHOLD - 1;
    }
}

I want the methods: hasInventoryOnline() and hasInventoryInNearbyStores() not sleep/block if another thread is running readInInventory(). Currently, I implemented the readInInventory() as synchronzed. However, this only ensures that one thread can access it at a given time. It does not prevent another [one] thread from processing hasInventoryOnline() invoked in parallel. Does it?

How do I lock access my storeInventory and onlineInventory ArrayLists so hasInventory... waits for readIn to complete?

Answer 1

What you've written does nothing at all, except 'pipeline' multiple different threads all trying to simultaneously invoke readIn.

Specifically, if you have a field that is read/written to by multiple threads, there are only two options (and remember, there are fields inside e.g. ArrayList and friends!):

  1. You carefully manage access to this field using volatile, synchronized, or other locking / concurrency mechanisms such as what you find in the java.util.concurrent package, for all threads that access it, not just the writing threads, also the ones that read, or

  2. Your code is extremely badly broken, in that it is just flat out broken, but, not in a testable fashion. It may work reliably all day today, and always on your testing hardware, and then fail next week on production. It's a bug, and a truly disastrous one, easily worth a thousand simpler bugs, because it is difficult to detect, and difficult to fix.

As you might imagine, the threat of #2 is such that you should in general just not write any concurrent code at all. Instead, focus on concurrency that avoid accessing the same fields altogether (e.g. a fork/join algorithm, or a map/reduce algorithm), or use systems that handle concurrency of data in different ways. For example, send all concurrent-access-required data through a database, using a SERIALIZABLE transaction level, and using JDBI or JOOQ with dbrunners to streamline the retries.

If you find that option 2 truly is the right move, trying to then handroll this using synchronized and/or volatile truly requires that you are an expert at the Java Memory Model and writing concurrent code: After all, writing a test to discover that you've messed up is incredibly difficult, as it would depend on your hardware, your OS, your specific JVM implementation, and the phase of the moon. Or if not the phase of the moon, the song playing in your music player right now. Such is the nature of race conditions.

On to your code

The reads aren't guarded by synchronized, thus you fail the rule and this code is fundamentally broken. Go back to the drawing board.

The fix

You have many options.

The simplest one is a rotate-into-place model: The readIn method should not be touching any shared state whatsoever: It should build the entire model of what it reads in, into its private state (local variables, for example: Stuff you do not share with other threads at all), and only when the job is complete, 'rotate into place': Update a shared-state field, with appropriate synchronized / volatile guards in place.

To other threads, in one instant that field is pointing to a valid state, and the next instant, it is pointing to a different also valid state. At no point is that state ever half-baked, avoiding the need to have to wait for it.

Remember, all object variables in java are references (pointers):

private List<String> data; // a field
void read() {
    List<String> local = new ArrayList<>();
    // take AGES to read in all that data, stuff it in local.
    synchronized (someLock) {
        data = local; // this is near-instant.
    }
}

data = local is incredibly fast, even if that list has millions of entries. You're not copying the millions of entries, you're just copying a pointer to it. 64 bits or so.

Remember, any attempt to read data must be guarded. So, your read code does the same in reverse, turn shared state into local first, so that you are then free from the shackles of the JMM that make interacting with shared state so very very tricky:

void doStuff() {
    List<String> local;
    synchronized (someLock) {
        local = data;
    }
}

In this specific case you may simply make that data field a volatile one, but there are plenty of cases where it isn't that easy.

In a realistic scenario, all of this is a moot point: This would be done with databases, not by reading in a file and saving that in memory.

Answer 2

Here is some code that demonstrates what you need to do. Note that it is not necessary to lock anything in the constructor (due to JMM happens before ordering rules).

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
public class InventoryService {
    private final ArrayList<String> myList;
    private final ReadWriteLock readWriteLock;
    public InventoryService()
    {
        myList = new ArrayList<>();
        readWriteLock = new ReentrantReadWriteLock();
    }
    public void add(String x)
    {
        // Exclusive lock.
        Lock lock = readWriteLock.writeLock();
        lock.lock();
        try
        {
            myList.add(x);
        }
        finally
        {
            lock.unlock();
        }
    }
    public String getFirst()
    {
        // Shared lock.
        Lock lock = readWriteLock.readLock();
        lock.lock();
        try
        {
           return myList.isEmpty()? null: myList.get(0);
        }
        finally
        {
            lock.unlock();
        }
    }
}
Rent Charter Buses Company
READ ALSO
Webview google login not showing accounts on device

Webview google login not showing accounts on device

I have a progressive web app and I'm trying to create a simple app for it that has a WebView that is showing the PWA

165
Iterate through JSON String in PHP?

Iterate through JSON String in PHP?

it seems to be a really easy question, but I am a little bit struggling: I am receiving a JSON String via JavaScriptNow I would like to iterate through the element

158
Clean code best practice in PHP. Less DB calls or friendlier code [closed]

Clean code best practice in PHP. Less DB calls or friendlier code [closed]

Want to improve this question? Update the question so it can be answered with facts and citations by editing this post

150