Is my class thread safe? Help!

Hi all,
I'm pretty inexperienced with threads and was wondering if I've handled this code correctly. The IPCollection class will be accessed by three different servlets, quite possibly at the same time. Are the two Vectors thread safe? Is the static modifier on the Vectors a problem? Any help is very much appreciated!
public class IPCollection
implements java.io.Serializable
{
private static Vector accepted;
private static Vector denied;

public void addAcceptedBooth(PollingBooth booth)
{
accepted.add(booth.getIpAddress());
}
public void addDeniedBooth(PollingBooth booth)
{
synchronized (this)
{
denied.add(booth.getIpAddress()); accepted.remove(booth.getIpAddress());
}
}
}

All methods of Vector are synchronized, so your code looks OK. It's necessary to be careful of deadlocks when you have more than one level of synchronization, but I don't see a problem here.

hi

why are the accepted and denied Vectors static ?
and i guess there are some more methods accessing these Vectors.

k

Originally posted by Gary McGath:
All methods of Vector are synchronized, so your code looks OK.

Big flashing warning lights.
The obsolete Vector class is synchronized, yes, but that rarely ever means that the code using it is threadsafe; in fact, it usually means that the code using it is more likely to be thread-unsafe because developers inexperienced in concurrent programming may easily be lured into a false sense of security.
In this case, I see a composite operation (addDeniedBooth). It is synchronized on IPCollection, but that will not prevent a concurrent call on addAcceptedBooth() coming through. This might not be a problem as things stand, but as the class grows more complex you may find that race conditions can cause your class to end up in an inconsistent state.
My advice would be: junk the Vectors and replace them by unsynchronized ArrayLists or, probably even better, HashSets. Use instance rather than static variables -- implement a Singleton pattern if you must. Synchronize both addAcceptedBooth and addDeniedBooth methods.
That way, you ensure that all operations on the IPCollection are atomic with respect to each other. You also avoid acquiring multiple monitor locks, which can indeed lead to deadlocks under some circumstances (although not in the code shown above). Your code will be faster and safer, and locking much simpler and easier to read.
- Peter

You can also use

I do not see where you are creating your Lists, so be very careful about that because it could pose problems in multithreaded environments.