views:

92

answers:

4

I am extending a library to do some work for me. Here is the code:

public static synchronized  String decompile(String source, int flags,UintMap properties,Map<String,String> namesMap)
    {
        Decompiler.namesMap=namesMap;
        String decompiled=decompile(source,flags,properties);
        Decompiler.namesMap=null;
        return decompiled;

    }

The problem is that namesMap is static variable. Is that thread safe or not? Because if this code runs concurently namesMap variable may change. What can I do for this?

+5  A: 

The method decompile is thread-safe (it will never be running on two threads concurrently), but if anything other than that method also uses the namesMap, then no, overall this is not thread-safe: Another method other than decompile running on a different thread could modify the map while the decompile method is using it, presumably causing mayhem. :-)

You might look at the classes in the java.util.concurrent namespace (for instance, ConcurrentHashMap) to see if any of those are applicable to what you're doing.

Edit (Responding to your comment.) If the static member namesMap is only ever used by decompile and not by anything else (you don't get a reference to it to anything else, etc.), then you're fine. The fact it's a static doesn't matter, if the only place it's used is serialized.

T.J. Crowder
And that "*only ever used*" means that one day, another developer will forget about this implicit requirement and use it elsewhere and you'll get spurious errors.
Aaron Digulla
If the static member `namesMap` is only used in one place, why not just change it from a static member to a method parameter? This way you can remove all doubt.
matt b
@Aaron: Absolutely.
T.J. Crowder
@matt: He has to store it *somewhere*. :-) Java doesn't have C's behavior with static local variables. (I'm not saying it's a bad thing it doesn't, just that it doesn't.)
T.J. Crowder
@T.J. What I meant was instead of storing the reference in a static var so that the other `decompile()` method can access it, it would make more sense to simply change the other `decompile()` method to accept this Map as a parameter
matt b
@allfinally i added the extra parameter and overloaded the method...
Parhs
+1  A: 

If there is a chance that other threads will make changes to namesMap while the method decompile(String, int, UintMap, Map) as described above is running, then you should be making a copy of the map passed in rather than just assigning a reference.

Decompiler.namesMap= new HashMap<String, String>(namesMap);

If there is a chance that other threads will make changes to the elements contained within the map, and not just the structure of the map itself, then you should make sure that your decompile() method and the other threads using namesMap are guarded by the same lock.

matt b
A: 

Actually Decompiler.namesMap=namesMap; is the only place to set namesMap

But nowhere else in the code the namesMap is changed.. Only read...

I want to ensure that

String decompiled=decompile(source,flags,properties);

would use the same namesMap.

Parhs
A: 

There will only be one namesMap so you don't have to worry whether it would use the same namesMap. It would.

Winter