Thread issue

[Mattias Ahlin]

I have a class named FoodListDay which represents all the meals for a day.

The Set foodListMeals is not supposed to contain any null elements. But since the attribute is set through setFoodListMeals I don't know what kind of Set is assigned to foodListMeals. If it is a HashSet for instance, null is a valid element type.

To fix this I introduced one line of code in setFoodListMeals that removes any null elements. But I fear that there might be some threading issues with this approach:

Thread A calls setFoodListMeals with a Set containing a null element. It executes the first line of code (assigns the Set to the attribute foodListMeals). It gets preempted by thread B. Thread B calls getFoodListMeals and gets back the set with the null element.

I'm not sure if making the method setFoodListMeals synchronized would take care of that because I don't completely understand thread/synchronization issues.

If thread B wants to call getFoodListMeals when thread A is executing setFoodListMeals, would making setFoodListMeals synchronized make thread B to wait until thread A is done (even though thread B doesn't try to access the synchronized method)?
Do I perhaps have to synchronize both methods to avoid any thread from accessing getFoodListMeals while another thread is already executing setFoodListMeals?

public class FoodListDay {
    private Set foodListMeals;
 
    public Set getFoodListMeals() {
        return foodListMeals;
    }
 
    public void setFoodListMeals(Set foodListMeals) {
    this.foodListMeals = foodListMeals;
    this.foodListMeals.remove(null);    // suggested fix.
    }
}

[Stan James]

Do I perhaps have to synchronize both methods to avoid any thread from accessing getFoodListMeals while another thread is already executing setFoodListMeals?

Yes.

As one of our moderators likes to point out, a synchronized method exposes the whole object as the handle for synchronization. That can be a problem if somebody else synchronizes on the same object instance and never lets go. One way to keep your handle private is to synchronize on the Set itself.

public Set getFoodListMeals() 
{
    synchronized(foodListMeals)
    {
        return foodListMeals;
    }
}

This documents exactly why you are synchronizing and doesn't block other methods that synchronize on something else. Seems like an all around good thing.
[ April 27, 2007: Message edited by: Stan James ]