CODEWITHSUNDEEP
javamultithreading
Vasu
Vasu

Reputation: 22402

synchronizedList with multiple threads

I have got the below sample code (uses Java), which will be accessed from multiple threads.

public class ApplicationProducts {

    private final List<Product> myAppProducts = Collections.synchronizedList(new ArrayList<>());

    private String firstPositionProductName;
    private int firstPositionProductAvailability;

    public void updateProducts(List<Product> products) {
        myAppProducts.addAll(products);
    }

    public boolean checkAndReplaceFirstProduct(Product product) {
        synchronized(myAppProducts) {
            if((product.getId()).equals(myAppProducts.get(0).getId())) {
                //calculations goes here to derive the below details
                firstPositionProductAVailability = 10;
                firstPositionProductName = abc;
                return true;
            } else {
                return false;
            }
        }
    }

    public boolean removeFirstProduct(Product product) {
        synchronized(myAppProducts) {
            if((product.getId()).equals(myAppProducts.get(0).getId()) &(firstPositionProductName.getId().equals(myAppProducts.get(0).getId())) {
                myAppProducts.remove(0);
                return true;
            } else {
                throw new IllegalStateException(" Lock taken by some other product : product can't be removed ");
            }
        }
    }
}

I have got the below queries, could you please help ?

(1) Is the above class thread safe ?

(2) Do I really need 'synchronization' on 'myAppProducts' as I am already checking for some condition i.e., will this class be thread safe after removing 'synchronized' from both methods ? One important point is that in my application, each thread calls checkAndReplaceFirstProduct() first and if the result is true (means acquired the product and does some calculations) and then only calls removeFirstProduct() ?

(3) Do I need to add 'synchronization' inside updateProducts() as well, which simply appends the more products to the existing list ?

(4) I know that synchronizedList's Iterator should be part of 'synchronized' block (https://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#synchronizedList(java.util.List), does it require the lock (synchronization) for accessing the 'synchronizedList.get(index)' method (like in above) ?

Upvotes: 2

Views: 677

Answers (2)

Dirk
Dirk

Reputation: 31053

Yes, you need the synchronized blocks, but not for any reason related to myAppProducts itself: observe your usage of firstPositionProductAVailability and firstPositionProductAvailability. As it is now, access to those members is protected by the synchronized blocks. If you remove those blocks, you'd introduce a potential race condition with respect to those members!

Accessing myAppProducts alone does not require the synchronized blocks. All method calls on the wrapped collection are properly interlocked by the wrapper, with the notable exception of iteration (as is called out explicitly in the documentation).

In general, you will need the synchronized blocks, if

  • you intend to iterate over the collection via for/iterator
  • you need to implement some action, which is non-atomic with respect to method calls on the collection (add an item only, if it is not already in the list, etc.), i.e., if a single logical "transaction" on the collection must be implemented using more than a single method call on the wrapper
  • you have some other state to protect (as is the case here with your other class members)

Regarding your edited

(2) Do I really need 'synchronization' on 'myAppProducts' as I am already checking for some condition i.e., will this class be thread safe after removing 'synchronized' from both methods ? One important point is that in my application, each thread calls checkAndReplaceFirstProduct() first and if the result is true (means acquired the product and does some calculations) and then only calls removeFirstProduct() ?

Here, you have a real race condition, which you cannot easily solve without changing the interface and/or with explicit cooperation by the callers of those methods. A possible solution would be to expose the object, on which we need to synchronize, so that the client of this class can to the right thing:

public Object getSynchronizationObject() {
    return myAppProducts;
}

and document, that your clients have to do

synchronized (obj.getSynchronizationObject()) {
    if (obj.checkAndReplaceFirstProduct(something)) {
        obj.removeFirstProduct(something);
    }
}

I mention this (but I do not recommend it!) since this is the approach taken by Java's Collections.synchronizedXxx with respect to iteration. A better solution would be to rework the interface, but I don't know anything about your actual use case, so I find it hard to come up with a proposal here.

Upvotes: 2

assylias
assylias

Reputation: 328568

Is the above class thread safe ?

Yes it looks fine. But I would add some checks before accessing myAppProducts.get(0) and myAppProducts.remove(0) (what if the list is empty?).

Do I really need 'synchronization' on 'myAppProducts' as I am already checking for some condition i.e., will this class be thread safe after removing 'synchronized' from both methods ?

Yes you do based on your code design:

  • for atomicity: both methods have "check-then-act" statements - if you remove the synchronization, the if condition may be true when you test it but become false by the time you reach the following line
  • for visibility: you assign values to the firstXxx variables and may read them from a different thread - without synchronization the result of the read is undefined.

Do I need to add 'synchronization' inside updateProducts() as well, which simply appends the more products to the existing list ?

No addAll is synchronized.

I know that synchronizedList's Iterator should be part of 'synchronized' block. Do I need to use a lock when calling 'synchronizedList.get(index)'?

No you don't, get is already synchronized.

Upvotes: 2

Related Questions