Win a copy of Cross-Platform Desktop Applications: Using Node, Electron, and NW.js this week in the JavaScript forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic

Does it cost problem?  RSS feed

 
Ronnie Ho
Ranch Hand
Posts: 47
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi all,

In a multithreading environment, does the following shared object generate concurrency problem or do I need to synchronized the method? Thanks.



[ October 25, 2005: Message edited by: Ronnie Ho ]
[ October 25, 2005: Message edited by: Ronnie Ho ]
 
Ken Blair
Ranch Hand
Posts: 1078
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
No, it's not safe. Synchronizing the methods will keep your class from accessing it concurrently from different threads. It won't keep other classes from getting a reference to it and accessing it concurrently. Unless there's a compelling reason not to, make the map private. If you synchronize the methods and make the amp private that should make accessing it thread-safe assuming there's no way for another class to get a reference to it and there's no other code that accesses it in your class.

Note I said it would make accessing the map safe. You still have Object obj which is added to the map and could be accessed concurrently. This may or may not be a concern for you. Since it's not a key, it shouldn't cause problems with the map, so the map itself should be okay.

Furthermore, you should take a look at java.util.concurrent.ConcurentHashMap if you're using 5.0. It's a hash table designed for concurrency that's part of the concurrent package just added.
 
Ronnie Ho
Ranch Hand
Posts: 47
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ken, thanks for your post. I did intend to make the HashMap private and I carelessly left it out (I just edited the post just now). My question is if I synchronized the add() method, but not the getAll() method, is it going to cost problem? If one thread is putting a new Entry into the map, while another trying to create an ArrayList object with the map.values(), is it going to corrupt the newly created ArrayList object ? Thanks.
 
Scott Selikoff
author
Bartender
Posts: 4087
21
Eclipse IDE Flex Google Web Toolkit
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
My instinct for this is no its not safe and map.values() could get corrupted, although questions like this always kind of made my head spin.

As for performance costs, you can improve concurrency in the getAll() by not sychronizing the entire method, but just the part that reads from the map.

 
Henry Wong
author
Sheriff
Posts: 23275
125
C++ Chrome Eclipse IDE Firefox Browser Java jQuery Linux VI Editor Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Originally posted by Scott Selikoff:
My instinct for this is no its not safe and map.values() could get corrupted, although questions like this always kind of made my head spin.

As for performance costs, you can improve concurrency in the getAll() by not sychronizing the entire method, but just the part that reads from the map.



Your instincts are correct in that it is not thread safe. The add() operation, in another thread, can change the internal structure of the collection, so even using the collection without changing it, will require synchronization.

Unfortunately, your attempt at improving performance is *not* thread safe. The values() method returns a collection backed by the original map. It too, needs to be synchronized, even during the small period while it is being loaded into the array list.

Henry
 
Ken Blair
Ranch Hand
Posts: 1078
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Originally posted by Ronnie Ho:
Ken, thanks for your post. I did intend to make the HashMap private and I carelessly left it out (I just edited the post just now). My question is if I synchronized the add() method, but not the getAll() method, is it going to cost problem? If one thread is putting a new Entry into the map, while another trying to create an ArrayList object with the map.values(), is it going to corrupt the newly created ArrayList object ? Thanks.


Yes it will cause a problem. There's nothing to stop a new value from being added while the ArrayList is being created. If you synchronize both and the conditions I set forth before are met it should be fine.



There's not much more you can do with it to improve performance. About the only thing that could be removed from a critical section is the creation of the Integer. Hardly worth the trouble. What might improve performance is the ConcurrentHashMap I mentioned. I've read it's designed to offer better performance and scalability when the map is expecte to be accessed by different threads often. With something this simple I doubt performance is going to be an issue anyway, but with something larger in scale being accessed by many threads a lot it might have a big impact.
 
Scott Selikoff
author
Bartender
Posts: 4087
21
Eclipse IDE Flex Google Web Toolkit
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Unfortunately, your attempt at improving performance is *not* thread safe. The values() method returns a collection backed by the original map. It too, needs to be synchronized, even during the small period while it is being loaded into the array list.


Hmm, I wasn't sure on this one. I tended to hope calling a method like map.values() would not expose the internal mapping of the set, ergo it is completely independent of the original mapping, but after reading the API, yeah it is backed, good catch.
 
Mr. C Lamont Gilbert
Ranch Hand
Posts: 1170
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Originally posted by Scott Selikoff:


Hmm, I wasn't sure on this one. I tended to hope calling a method like map.values() would not expose the internal mapping of the set, ergo it is completely independent of the original mapping, but after reading the API, yeah it is backed, good catch.


Im missing the point here. while map.values is still a part of the original map, the new ArrayList will be its own list with no association. The safety or lack thereof does not depend on where the duplication occurs.

If you use a synchronized map you can avoid these issues. Though you can probably synchronize on a broader scope and gain more value.

My gut feeling is that the collections are not corruptable, but that you can loose values in your add method with no synch.
 
Jim Yingst
Wanderer
Sheriff
Posts: 18671
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
[CLG]: Im missing the point here. while map.values is still a part of the original map, the new ArrayList will be its own list with no association. The safety or lack thereof does not depend on where the duplication occurs.

Yes, it does. After it's been created the new ArrayList will be independent, sure. However while it's being created (during the execution of the new ArrayList() constructor), it's iterating through the elements of the map.values() set. And if that iteration is not protected through synchronization, the getAll() method will not be safe.

[CLG]: If you use a synchronized map you can avoid these issues.

No, this is yet another example where a supposedly "thread-safe" collection is in fact, not. Like Vector and Hashtable before them, the Collections.synchronizedXXX() methods tend to give people a false sense of security. Individual method calls on a synchronized map may be thread-safe, but operations which require multiple method invocations (e.g. iterating through a collection) are not safe unless externally synchronized. As pointed out in the API for Collections.synchronizedMap() and related methods.

[CLG]: Though you can probably synchronize on a broader scope and gain more value.

Yes. Synchronizing on a broader scope is the solution here. Synchronizing the getAll() method, for example.

[CLG]: My gut feeling is that the collections are not corruptable, but that you can loose values in your add method with no synch.

I think the biggest risk is if a put() causes the HashMap to resize its table array. Another simulaneous put might drop its data somewhere it will never be found again; a simultaneous get() could fail to find an entry - not just a recently-added entry, but also one that's been there a long time. If removes() were added to the mix then things like NullPointerException could easily occur as well. Could be very hard to debug.
[ October 26, 2005: Message edited by: Jim Yingst ]
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!