java thread safe code + an atomic method question

Question

I have a class Manager that is going to be accessed by multiple threads at the same time, I want to know if I did it the right way ?
also I think I need RemoveFoo to be atomic, but I'm not sure

public class Manager
{
    private ConcurrentHashMap<String, Foo> foos;
    //init in constructor

    public RemoveFoo(String name)
    {
        Foo foo = foos.Get(name);
        foo.RemoveAll();
        foos.Remove(name);
    }

    public AddFoo(Foo foo)
    {...}
}

public class Foo
{
    private Map<String,Bar> bars;
    //intialize it in constructor

    //same for RemoveBar
    public void AddBar(Bar bar)
    {
        synchronized(this)
        {
            bars.put(bar.id, bar);
        }
    }

    public void RemoveAll()
    {
        synchronized(this)
        {
            //some before removall logic for each one
            bars.remove(bar.id, bar);
        }
    }

}

public class Bar
{}

Answer 1

RemoveFoo could be problematic. I suggest to use:

Foo foo = foos.remove (name);
if (foo != null) foo.removeAll();

instead. This makes sure that the map doesn't change between get() and remove().

In Foo, it's enough to synchronize on bars instead of the whole instance. But that's just a minor optimization.

Answer 2

You do not need synchronised methods as you are using a ConcurrentHashMap, however be aware that Foo foo = foos.Get(name) could return null as another thread could already have removed the entry from the map.

Members can be declared as Map<String, Foo> foos, but must be initialsed as foos = new ConcurrentHashMap<String, Foo>;

Answer 3

Declare RemoveFoo(String) as synchronized:

public synchronized void RemoveFoo(String name) {
    …
}

Also, be advised of the following:

• method names should be lower case, e.g. removeFoo instead of RemoveFoo. This is not C#. :)
• Every method needs a return type: public removeFoo() is not a valid method declaration, it needs to be public void removeFoo().

Answer 4

If you use a concurrentHashMap in Foo like

private Map<String,Bar> bars = new ConcurrentHashMap<String, Bar>();

maybe you can do away with the synchronization in Foo as well.

Answer 5

I am not sure what you are going to do on Foo and Bar, but it looks like a pattern of deallocation.

If they are not referenced by others, just call foos.Remove(name); and let GC engine handle the deallocation.