• Post Reply Bookmark Topic Watch Topic
  • New Topic

Is my class thread safe? Help!  RSS feed

 
Stephanie Spike
Greenhorn
Posts: 11
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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());
}
}
}
 
Gary McGath
Ranch Hand
Posts: 52
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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.
 
karl koch
Ranch Hand
Posts: 388
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
hi

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

k
 
Peter den Haan
author
Ranch Hand
Posts: 3252
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
 
Mr. C Lamont Gilbert
Ranch Hand
Posts: 1170
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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.
 
Don't get me started about those stupid light bulbs.
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!