aspose file tools*
The moose likes Threads and Synchronization and the fly likes is this class thread-safe ? Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


Win a copy of Java 8 in Action this week in the Java 8 forum!
JavaRanch » Java Forums » Java » Threads and Synchronization
Bookmark "is this class thread-safe ?" Watch "is this class thread-safe ?" New topic
Author

is this class thread-safe ?

Edward Chen
Ranch Hand

Joined: Dec 23, 2003
Posts: 798
Iam learning the thread in JDK1.5.

The testing scenario is : we have 10 users and create 10 threads to authorize them. If authorized, put it into a ConcurrentHashMap, then the other threads don't authorize it again.

Here is the coding.

Questions:
1. do you think it is thread-safe ?
2. "static final" ConcurrentHashMap is necessary ? or "static" is enough ?
3. do we have any tool to test multithread application ?

Jim Yingst
Wanderer
Sheriff

Joined: Jan 30, 2000
Posts: 18671
[Edward]: 1. do you think it is thread-safe ?

No, it isn't. There are several places where you have mutable data that is accessed by more than one thread with no protection. Specifically, the field AuthUser.user, and the fields User.userName and User.password. They are set in one thread and accessed in another. In theory the access "should" always happen after the initial setting - but this is the sort of thing where really weird stuff can happen with multithreading. It's not guaranteed.

For each of these fields, the basic options are: (a) synchronize access, or (b) make them immutable. In both cases I'd favor the second. For AuthUser.user, that's really simple - just declare it final. That, combined with being initialized in the constructor, is enough to guarantee that any other threads who see the AuthUser object will see it with its field properly initialized. For User.useName and User.password, you can do much the same thing, making both final. But then you have to change the class to ensure that both fields are set in the constructor, not after the constructor. If you don't want to do that, then you could instead synchronize each mehtod that accesses those fields. Or, you could also make both fields volatile it you like.

[Edward]: 2. "static final" ConcurrentHashMap is necessary ? or "static" is enough ?

Definitely make it final. (Just like the other fields which I recommended you add final to.)

[Edward]: 3. do we have any tool to test multithread application ?

Mmmm, I don't know a good answer to this one. I usually would create a stress test using JUnit (even though it's not a unit test). Make lots and lots of threads and have them bang on each other a lot. Unfortunately because threading problems are unpredicatble in nature it may be impossible to create truly reliable tests here - but something is better than nothing, I guess. Maybe others will have better answers. Or maybe this should be a separate thread in the Testing forum.


"I'm not back." - Bill Harding, Twister
Edward Chen
Ranch Hand

Joined: Dec 23, 2003
Posts: 798
Originally posted by Jim Yingst:
[Edward]: 1. do you think it is thread-safe ?

No, it isn't. There are several places where you have mutable data that is accessed by more than one thread with no protection. Specifically, the field AuthUser.user, and the fields User.userName and User.password. They are set in one thread and accessed in another. In theory the access "should" always happen after the initial setting - but this is the sort of thing where really weird stuff can happen with multithreading. It's not guaranteed.

For each of these fields, the basic options are: (a) synchronize access, or (b) make them immutable. In both cases I'd favor the second. For AuthUser.user, that's really simple - just declare it final. That, combined with being initialized in the constructor, is enough to guarantee that any other threads who see the AuthUser object will see it with its field properly initialized. For User.useName and User.password, you can do much the same thing, making both final. But then you have to change the class to ensure that both fields are set in the constructor, not after the constructor. If you don't want to do that, then you could instead synchronize each mehtod that accesses those fields. Or, you could also make both fields volatile it you like.


1. I add 'final' to varible and get/set method. is this what you mean ?


2. I didn't put 'final' there, because I think the 'user' is thread confined, it is only visible to each thread and therefore it could NOT be shared among threads. am I correct ?

Thanks.
Peter Chase
Ranch Hand

Joined: Oct 30, 2001
Posts: 1970
The rule with fields (instance or static) is that you should always declare them "final" unless they actually need to be modified after construction.


Betty Rubble? Well, I would go with Betty... but I'd be thinking of Wilma.
Jim Yingst
Wanderer
Sheriff

Joined: Jan 30, 2000
Posts: 18671
[Edward]: 1. I add 'final' to varible and get/set method. is this what you mean ?

Yes - you added final to the one variable, AuthUser.user, and added synchronization to the get and set for the other two variables, User.userName and User.password. I believe your code is now thread-safe. I haven't analyzed it really, reallly carefully, or tested it, but it looks good.

[Edward]: 2. I didn't put 'final' there, because I think the 'user' is thread confined, it is only visible to each thread and therefore it could NOT be shared among threads. am I correct ?

No - user is definitely set by one thread (main) and read by a different thread (running AuthUser.run()). It does seem like the read will only come after the constructor completes, but it's not guaranteed because the JVM can do strange things with multiple threads. See JLS 17.5: Final Field Semantics. In particular see the discussion of the class FinalFieldExample, where variable x is thread-safe, but y is not, because y was not marked final. That's why it's good to mark every field final unless you need it to change, as Peter says.
Edward Chen
Ranch Hand

Joined: Dec 23, 2003
Posts: 798
Originally posted by Jim Yingst:

No - user is definitely set by one thread (main) and read by a different thread (running AuthUser.run()). It does seem like the read will only come after the constructor completes, but it's not guaranteed because the JVM can do strange things with multiple threads.

I think we are talking about these lines


On above coding, in line-A we only have a main thread, not multithreads. so I think no synchronization problem.

And more, in line-B, each thread has its own different object (auth1), still not shared. In my understanding, when we 'new' an object, that object has its own memory chunk, then that thread will work on that memory chunk, no any overlapping, therefore it is safe.

Let me have this question: how could I confine an object to a thread to achieve safety ? Could you please modify the above coding as an example ?

Thanks for your patience and help.
Jim Yingst
Wanderer
Sheriff

Joined: Jan 30, 2000
Posts: 18671
[Edward]: On above coding, in line-A we only have a main thread, not multithreads. so I think no synchronization problem.

There's no synchronization problem yet...

[Edward]: And more, in line-B, each thread has its own different object (auth1), still not shared. In my understanding, when we 'new' an object, that object has its own memory chunk, then that thread will work on that memory chunk, no any overlapping, therefore it is safe.

But the new AuthUser is shared between two different threads, the moment you call start() which starts a new thread that uses the object. It was created in one thread, and used in another. Yes, I know that none of the other new threads are using the same object, but there are still two threads that use every object: main, and the new thread just created.

[Edward]: Let me have this question: how could I confine an object to a thread to achieve safety ? Could you please modify the above coding as an example ?

Offhand I don't see a good way to confine the AuthUser to one thread, since you need to create it before you start the new thread. Maybe there's a way, but I'm not seeing it because I'm stuck on the really easy solution that I already suggested: make the AuthUser.user field final. That way it doesn't matter that the user is shared between threads, because the user doesn't change.
Edward Chen
Ranch Hand

Joined: Dec 23, 2003
Posts: 798
Originally posted by Jim Yingst:

but there are still two threads that use every object: main, and the new thread just created.

This really clear my question.

I don't like to put 'synchronized' in every setter/getter method. So I modify the coding, add synchronized block, is it still thread-safe ?
Jim Yingst
Wanderer
Sheriff

Joined: Jan 30, 2000
Posts: 18671
[Edward]: I don't like to put 'synchronized' in every setter/getter method. So I modify the coding, add synchronized block, is it still thread-safe ?

Not the way you've shown. The main thread is still using setUserName() and setPassword() with no synchronization.

Also, I didn't notice it before but there's a problem with the contains() method. This will call the equals() method of User, which has not been overridden, so it's the method inherited from Object, which is equivalent to ==. This method is thread-safe, but I suspect that it's not what you want. You may well need to override equals() (and hashCode()) to return something that depends on userName and password - and if you do, you need synchronization there too. Using the ConcurrentHashMap doesn't help in this respect, as it takes care of its own thread safety, but not the thread safety of the equals() method for another class. So you'd need synchronization inside the equals() method.

Actually I suspect you don't want to use contains() at all, as it's fairly inefficient. Since it's checking values rather than keys, it must check the entire list of values, without using the hashcode at all. You will get much better results by using containsKey() - and since the key is a String, which is immutable, it's already thread-safe. So if you change contains() to containsKey() you can ignore my previous paragraph.

The problem with using syncronized blocks as you have is that it puts the responsibility for synchronizing the code on classes other than the one that needs the synchronization. That's possible to do (and I certainly have been known to do it myself), but it's risky since the person writing the class that uses User isn't necessarily as familiar with User as the person who wrote User in the first place. Even if it's you in both cases - as your project gets bigger and you have to revisit your own code weeks or months later, you may well find that you don't remember the details as well as you'd like. So if there's a way to make User thread-safe internally, that's usually safer. In this case, I think (again) that the best way to do that is to make the class immutable:

Now you can't use User in a thread-unsafe manner, (well, not without reflection), and there's no synchronization involved. Much easier, safer, and faster.
 
Don't get me started about those stupid light bulbs.
 
subject: is this class thread-safe ?
 
Similar Threads
help looping through objects
Boolean and boolean problems in JavaBeans
help with this program! I am stuck
[solved] authorization with phaselistener problem
IBatis: boolean mapping misunderstanding