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 surepublic 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(thi开发者_Go百科s)
{
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
{}
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>;
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.
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 ofRemoveFoo
. This is not C#. :) - Every method needs a return type:
public removeFoo()
is not a valid method declaration, it needs to bepublic void removeFoo()
.
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.
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.
精彩评论