Issue accessing class variable from thread.

Question

Hello,

The code below is meant to take an arraylist of product objects as an input, spun thread for each product(and add the product to the arraylist 'products'), check product image(product.imageURL) availability, remove the products without images(remove the product from the arraylist 'products'), and return an arraylist of products with image available.

package com.catgen.thread;

import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;

import com.catgen.Product;
import com.catgen.Utils;

public class ProductFilterThread extends Thread{

    private Product product;
    private List<Product> products = new ArrayList<Product>();

    public ProductFilterThread(){
    }

    public ProductFilterThread(Product product){
        this.product = product;
    }

    public synchronized void addProduct(Product product){
         System.out.println("Before add: "+getProducts().size());
         getProducts().add(product);
         System.out.println("After add: "+getProducts().size());
    }

    public synchronized void removeProduct(Product product){
         System.out.println("Before rem: "+getProducts().size());
         getProducts().remove(product);
         System.out.println("After rem: "+getProducts().size());
    }

    public synchronized List<Product> getProducts(){
        return this.products;
    }

    public synchronized void setProducts(List<Product> products){
        this.products = products;
    }

    public void run(){
        boolean imageExists = Utils.fileExists(this.product.ImageURL);
        if(!imageExists){
            System.out.println(this.product.ImageURL);
            removeProduct(this.product);
        }
    }

    public List<Product> getProductsWithImageOnly(List<Product> products){
        ProductFilterThread pft = null;
        try{
            List<ProductFilterThread> threads = new ArrayList<ProductFilterThread>();
            for(Product product: products){
                pft = new ProductFilterThread(product);
                addProduct(product);
                pft.start();
                threads.add(pft);
            }
            Iterator<ProductFilterThread> threadsIter = threads.iterator();
            while(threadsIter.hasNext()){
                ProductFilterThread thread = threadsIter.next();
                thread.join();
            }
        }catch(Exception e){
            e.printStackTrace();
        }
        System.out.println("Total returned products = "+getProducts().size());
        return getProducts();
    }
}

Calling statement:

displayProducts = new ProductFilterThread().getProductsWithImageOnly(displayProducts);

Here, when addProduct(product) is called from within getProductsWithImageOnly(), getProducts() returns the list of products, but that's not the case(no products are returned) when the method removeProduct() is called by a thread, because of which the products without images are never removed. As a result, all the products are returned by the module whether or not the contained products have images.

What can be the problem here?

Thanks in advance. James.

Answer 1

You're creating a new ProductFilterThread each iteration in the loop:

new ProductFilterThread(product);

Each ProductFilterThread has its own

private List<Product> products = new ArrayList<Product>();

So when you, in the run method, do

removeProduct(this.product);

you're removing the product from it's own products instance.

I suggest you design it differently, by perhaps giving the products-list the thread should work with, as argument to the ProductFilterThread:

private List<Product> products;

public ProductFilterThread(List<Product> products) {
    this.products = products;
}

But I must say that you should think carefully when working with multiple threads like this. You need to synchronize the access to the data structures carefully.

Answer 2

If you want you products list to be shared by all Filter Threads then you'll have to make it static. Fields are confined to a single thread.

Another remark - maybe you should rewrite your getter/setter methods for products to export an unmodifiable list and to read the input in a newly created list. Otherwise this can be the cause for multiple headaches in this multithreaded design (just imagine another thread 'getting' the list and modifying it while the filtering is active..)