CODEWITHSUNDEEP
javamultithreadingjsfconcurrencythread-safety
Rajat Gupta
Rajat Gupta

Reputation: 26617

Thready safety of Maps, used privately inside application scoped bean & only for ready purposes after once building

I have an @ApplicationScoped bean for all users, that stores the ids-> names & vice versa in Trove & java.util maps.

I just build the maps once at construction of bean or (in case of manual refresh by the website admin).

Inside the bean methods, I am just using the get() with the maps, so not modifying the map. Is this going to be thread safe since it is used only for ready purposes? I am not sharing the maps with any other beans outside & not modifying the maps(adding/removing entries) anytime in my code.

Also, Is it neccesary in this case to make the fields final ?

Bean code as follows:

@ApplicationScoped
@ManagedBean(name="directory", eager=true)
public class directory {

    private static TIntObjectHashMap<String> idsToNamesMap;
    private static TreeMap<String, Integer> namesToIdsMap;

    @PostConstruct
    public void buildDirectory(){
        // building directory here ....
    }

    public String getName(int topicId){
        return idsToNamesMap.get(topicId);
    }    

    public List<Entry<String, Integer>> searchTopicsByName(String query){
        return new ArrayList(namesToIdsMap.subMap(query, true, query+"z", true).entrySet());
    }        
}

Upvotes: 1

Views: 572

Answers (2)

Eugene Retunsky
Eugene Retunsky

Reputation: 13139

You don't have to declare them volatile or protect with any kind of synchronization in this case. As long as the constructing thread will build them and synchronize with the main memory.

For that the constructing thread need just to make a single write to a volatile variable or enter/exit a synchronization lock. This will pass a memory barrier and all local thread data will be in the main thread. Then it will be safe for all other threads to read this data.

Even more - unnecessary volatile or synchronization block - costs a serious performance penalty - on each access to the variable it will pass the memory barrier - which is an expensive operation

Upvotes: 3

sparc_spread
sparc_spread

Reputation: 10843

There could be a visibility issue after the object is constructed. That is, in the immediate aftermath of your constructor calls, the maps may appear populated to the thread that populated them, but not necessarily to other threads, at least not right away. This type of issue is extensively discussed in chapter 3 of Java Concurrency in Practice. However, I think that if you declare the maps as volatile: 

private static volatile TIntObjectHashMap<String> idsToNamesMap;
private static volatile TreeMap<String, Integer> namesToIdsMap;

You should be OK.

Update

I just realized something while looking at your code again. The maps are static - why are they being populated in an instance context by a constructor? First off, it is confusing to the reader. Second, if more than one instance of the object is created, then you will have additional writes to the maps, not just one, possibly while other threads are reading them.

You should either make them non-static, or populate them in a static initialization block.

Upvotes: 3

Related Questions