8

Will the following code cause same problems, if variable 'commonSet' of this method was instead a class level field. If it was a class level field, I'll have to wrap adding to set operation within a synchronized block as HashSet is not thread safe. Should I do the same in following code, since multiple threads are adding on to the set or even the current thread may go on to mutate the set.

public void threadCreatorFunction(final String[] args) {
    final Set<String> commonSet = new HashSet<String>();

    final Runnable runnable = new Runnable() {
        @Override
        public void run() {
            while (true) {
                commonSet.add(newValue());
            }
        }
    };

    new Thread(runnable, "T_A").start();
    new Thread(runnable, "T_B").start();
}

The reference to 'commonSet' is 'locked' by using final. But multiple threads operating on it can still corrupt the values in the set(it may contain duplicates?). Secondly, confusion is since 'commonSet' ia a method level variable - it's same reference will be on the stack memory of the calling method (threadCreatorFunction) and stack memory of run methods - is this correct?

There are quite a few questions related to this:

But, I cannot see them stressing on thread safe part of such sharing/passing of mutables.

Community
  • 1
  • 1
haps10
  • 3,496
  • 6
  • 32
  • 38
  • See also http://stackoverflow.com/questions/1299837/cannot-refer-to-a-non-final-variable-inside-an-inner-class-defined-in-a-differen – nos Aug 06 '12 at 07:45

6 Answers6

9

No, this is absolutely not thread-safe. Just because you've got it in a final variable, that means that both threads will see the same reference, which is fine - but it doesn't make the object any more thread-safe.

Either you need to synchronize access, or use ConcurrentSkipListSet.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
5

An interesting example.

The reference commonSet is thread safe and immutable. It is on the stack for the first thread and a field of your anonymous Runnable class as well. (You can see this in a debugger)

The set commonSet refers to is mutable and not thread safe. You need to use synchronized, or a Lock to make it thread safe. (Or use a thread safe collection instead)

Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
1

I think you're missing a word in your first sentence:

Will the following code cause same problems if variable 'commonSet' of this method was a ??? instead a class level field.

I think you're a little bit confused though. The concurrency issues have nothing to do with whether or not the reference to your mutable data structure is declared final. You need to declare the reference as final because you're closing over it inside the anonymous inner class declaration for your Runnable. If you're actually going to have multiple threads reading/writing the data structure then you need to either use locks (synchronize) or use a concurrent data structure like java.util.concurrent.ConcurrentHashMap.

DaoWen
  • 32,589
  • 6
  • 74
  • 101
1

The commonSet is shared among two Threads. You have declared it as final and thus you made the reference immutable (you can not re-assign it), but the actual data inside the Set is still mutable. Suppose that one Thread puts some data in and some other Thread reads some data out. Whenever the first thread puts data in, you most probably want to lock that Set so that no other Thread could read until that data is written. Does that happen with a HashSet? Not really.

Eugene
  • 117,005
  • 15
  • 201
  • 306
0

As others have already commented, you are mistaking some concepts, like final and synchronized.

I think that if you explain what you want to accomplish with your code,it would be much easier to help you. I've got the impression that this code snippet is more an example that the actual code.

Some questions: Why is the set defined inside the function? should it be shared among threads? Something that puzzles me is that you crate two threads with the same instance of the runnable

    new Thread(runnable, "T_A").start();
    new Thread(runnable, "T_B").start();
pablochacin
  • 947
  • 1
  • 14
  • 35
0

Whether commonset is used by single thread or multiple it is only the reference that is immutable for final objects(i.e, once assigned you cannot assign another obj reference again) however you can still modify the contents referenced by this object using that reference.

If it were not final one thread could have initialized it again and changed the reference commonSet = new HashSet<String>(); commonSet.add(newValue()); in which case these two threads may use two different commonsets which is probably not what you want

srikanth yaradla
  • 1,225
  • 10
  • 13