Modify outer object from inside a thread

Question

public void fooAndBar() {
    HashMap<Foo, Bar> fooBarMap = new HashMap<>();
    CompletionService completionService = new ExecutorCompletionService(exec);

    for(int i=0; i<10; i++) {
        completionService.submit(new Callable() {
            @Override
            public Void call() throws Exception {
                fooBarMap.put(new Foo(i), new Bar(i));
                return null;
            }
        });
    }
}

Is it safe to modify the HashMap inside the Callable?
Should the hashmap be final (or maybe volatile) and if so, why?
Should I use a structure other than HashMap, something like ConcurrentHashMap or SynchronizedMap and why?

I'm trying to grasp java concepts so please bear with me

score 2 · Accepted Answer · edited May 23 '17 at 12:02

Is it safe to modify the HashMap inside the Callable?

No. If you are using a threadpool I assume you are planning to have more of those callables running in parallel. Any time an object with mutable state is accessed from more than one thread, that's thread-unsafe. If you write to a thread-unsafe hashmap from two threads simultaneously, its internal structure will be corrupted. If you read from a thread-unsafe hashmap while another thread is writing to it simultaneously, your reading thread will read garbage. This is a very well known and extensively studied situation known as a Race Condition, a description of which would be totally beyond the scope of this answer. For more information, read about Race Condition on Wikipedia or on another question answered back in 2008: Stackoverflow - What is a Race Condition.

Should the hashmap be final (or maybe volatile) and if so, why?

For your purposes it does not need to be final, but it is always a good practice to make final anything that can be made final.

It does not need to be volatile because:

if you were to make it volatile, you would be making the reference to it volatile, but the reference never changes, it is its contents that change, and volatile has nothing to do with those.
the threadpool makes sure that call() will be executed after fooBarMap = new HashMap<>(). (If you are wondering why such a thing could ever be a concern, google for "memory boundary".)

Should I use a structure other than HashMap, something like ConcurrentHashMap or SynchronizedMap and why?

Definitely. Because, as I wrote earlier, any time an object with mutable state is accessed from more than one thread, that's thread-unsafe. And ConcurrentHashMap, SynchronizedMap, synchronize, etc. exist precisely for taking care of thread-unsafe situations.

Thanks. Can you please describe how being thread-unsafe could compromise the code if I'm only `writing` to the HashMap? Also I'm not sure if I understand why the HashMap doesn't need to be volatile (maybe because the reference held by `fooBarMap` never changes?), but I'll make sure to read more about it. One other thing to mention, in the logic that I'm writing, no other thread will read from fooBarMap until all threads are finished processing. So do I still need a `ConcurrentHashMap` in that scenario? — prettyvoid, Apr 17 '17 at 08:54
Correct, the reference to the map never changes, so no need to make it `volatile`. As for the rest, I just amended my answer. — Mike Nakis, Apr 17 '17 at 09:43
I agree with everything in this answer, but I think the question code needs to be altered a bit... Isn't all the provided implementation in the scope of a single method? In a single method with no class/instance variables there will be no shared state and thus no concurrency issues. — Thomas Kabassis, Apr 17 '17 at 10:45
@tomkab It is not within the scope of a single method. The method uses an `ExecutorCompletionService` to spawn a thread, which runs a child method, which attempts to access state of the parent method ***from a different thread***. And this is just an example, so it spawns only one thread, but in reality he'd be spawning many. — Mike Nakis, Apr 17 '17 at 11:31
Thanks again, I feel more comfortable with what I'm doing now. — prettyvoid, Apr 18 '17 at 12:30

score -4 · Answer 2 · answered Apr 17 '17 at 07:54

-4

Hashmap should not be final, as you are modifying it multiple times(from within a for loop).

If you make it final, you may get an error.

answered Apr 17 '17 at 07:54

Deepesh Choudhary

660
1
8
17

2

Since it is used inside of an anonymous inner class it *must* be final or effectively final – SilverNak Apr 17 '17 at 07:55
2

And also, it being final has absolutely nothing to do with whether you can modify its contents. – Mike Nakis Apr 17 '17 at 08:00

Modify outer object from inside a thread

2 Answers2