Checking for String value in this Collections.synchronizedSet(new HashSet()) ?

[Amandeep Singh]

I have a code in which i am making a synchronized set.

Set<String> setUPSPayerNums = Collections.synchronizedSet(new HashSet()) ;

Sun Documentation says-
It is imperative that the user manually synchronize on the returned set when iterating over it:

Set s = Collections.synchronizedSet(new HashSet());
      ...
  synchronized(s) {
      Iterator i = s.iterator(); // Must be in the synchronized block
      while (i.hasNext())
          foo(i.next());
  }
 

In my case, i am not iterating. I am checking for particular String in set using contains method, i do not have to use synchronized block.
like this-

return (setUPayerNums!= null?setUPayerNums.contains(sPayerUnitNum):false);

Am i following Sun's documentation advice and the above code is safe without using the synchronized block?

[Paul Clapham]

The advice you posted is about iterating over the Set. You aren't iterating over the Set so the advice doesn't apply. Which is what you thought, correct?

[Amandeep Singh]

1) Yes, the advice is about iterating. What i thought is- they just want to give a idea- it may apply to other scenario- when using the contains method.

2) If we are just checking through contains method without using a synchronized block. So what is the use of making a synchronizing a set? (Just to make sure, only 1 thread makes a set at a time.)

[Adi Pandit]

I believe you need to synchronize even if you are just calling setUPayerNums.contains(sPayerUnitNum).
Internally HashSet has a HashMap which goes through the entries in the relevant bucket to lookup the key passed. This means it is iterating a sub-set of the values. So if another thread is manipulating the Set you are not guaranteed the correct value and you could get some concurrency exceptions. I think the warning: "Failure to follow this advice may result in non-deterministic behavior." applies to the code you are trying.

[Paul Clapham]

Adi Pandit wrote:I believe you need to synchronize even if you are just calling setUPayerNums.contains(sPayerUnitNum).
Internally HashSet has a HashMap which goes through the entries in the relevant bucket to lookup the key passed. This means it is iterating a sub-set of the values.

No, that's wrong. The advice quoted in the original post is about writing code which iterates over the set. It doesn't require the reader to know the internal implementation of other methods and to determine whether they might be iterating over something. It simply means what it says, no more than that.

If the contains() method actually does iterate over the set, or part of it, then the internal code will synchronize accordingly. That's precisely what the synchronizedSet method is for. All you have to worry about is code that you write. You don't have to second-guess code written by the Java API coders.

[Henry Wong]

Amandeep Singh wrote:1) Yes, the advice is about iterating. What i thought is- they just want to give a idea- it may apply to other scenario- when using the contains method.

The set that is returned from the synchronizedSet() method returns a set that synchronizes all the method calls for you. The reason the iterator is a special case is because you don't want the set to change in-between many calls of the iterator, hence, you need to hold the lock to the set, for the full duration of the loop.

Amandeep Singh wrote:2) If we are just checking through contains method without using a synchronized block. So what is the use of making a synchronizing a set? (Just to make sure, only 1 thread makes a set at a time.)

Without using the synchronizedSet() method, then your set is not synchronized, and hence, you need to use the synchronized block for all the calls to the set. With the synchronizedSet(), then you only need to externally synchronize for the iterator, or any other operation that requires thread safety across mulitple calls to the set.

Henry

[Adi Pandit]

I guess my understanding was wrong. You do not need to synchronize when calling contains the wrapper class takes care of synchronizing on the internal lock being used when contains is invoked.

Code snippet from Collections.SynchronizedCollection

public boolean contains(Object o) {
	    synchronized(mutex) {return c.contains(o);}
        }

[Amandeep Singh]

Thanks Paul, Adi & Hengry.

[G.Sathish kumar]

i feel locking the synchronised set object while iteration because access the right data like when my iteration there in 3 index so if i have new object add in 2 indexposition does give the issue.

on contains method checking also kind of iteration to check each data so i feel synchronizing the set object while contins method is the best.

please update me if i am wrong.

[G.Sathish kumar]

sorry for the confusion just now i checked so contains methods internally synchronized so no need of locking