Read and write into shared thread variables

Question

first of all i am new to threads and shared variables. So please be kind with me ;-)

I'm having a class called Routing. This class recieves and handles messages. If a message is of type A the Routing-Object should pass it to the ASender Object which implements the Runnable Interface. If the message is of type B the Routing-Class should pass it to the BSender Object.

But the ASender and BSender Objects have common variables, that should be stored into the Routing-Object.

My idea now is to declare the variables as synchronized/volatile in the Routing-Object and the getter/setter also.

Is this the right way to synchronize the code? Or is something missing?

Edit: Added the basic code idea.

RoutingClass

public class Routing {

private synchronized Hashtable<Long, HashSet<String>> reverseLookup;
private ASender asender;
private BSender bsender;

public Routing() {
    //Constructor work to be done here.. 
    reverseLookup = new Hashtable<Long, HashSet<String>>();

}

public void notify(TopicEvent event) {

    if (event.getMessage() instanceof AMessage) {
        asender = new ASender(this, event.getMessage())

    } else if (event.getMessage() instanceof BMessage) {
        bsender = new BSender(this, event.getMessage())

    }
}

public synchronized void setReverseLookup(long l, Hashset<String> set) {
    reverseLookup.put(l, set);

}

public synchronized Hashtable<Long, Hashset<String>> getReverseLookup() {
    return reverseLookup;
}
}

ASender Class

public class ASender implements Runnable {

private Routing routing;
private RoutingMessage routingMessage;

public ASender(Routing r, RoutingMessage rm) {
    routing = r;
    routingMessage = rm;
    this.run();
}

public void run() {
    handleMessage();
}

private void handleMessage() {
    // do some stuff and extract data from the routing message object

    routing.setReverseLookup(somethingToSet)
}
}

We can't tell if the code is right without seeing the code. Code matters. — JB Nizet, Jul 17 '13 at 08:44
[Do I look like a Guru?](http://programmer.97things.oreilly.com/wiki/index.php/The_Guru_Myth) — Adam Arold, Jul 17 '13 at 08:46
I think you are good to go. Just avoid nested synchronization, and if you have to, always lock in the same order. Google how to avoid java deadlocks, e.g. Listing 1 [here](http://www.javaworld.com/javaworld/jw-10-2001/jw-1012-deadlock.html) shows how NOT to do it. — sotix, Jul 17 '13 at 08:57

score 1 · Accepted Answer · edited May 23 '17 at 11:49

Some comments:

Hashtable is a thread-safe implementation, you do not need another "synchronized" keyword see this and this for more information
Avoid coupling, try to work with interfaces or pass the hashtable to your senders, see this for more information
Depending on the amount of senders, you might want to use a ConcurrentHashMap, it greatly improves the performance, see ConcurrentHashMap and Hashtable in Java and Java theory and practice: Concurrent collections classes

This would conclude something like...:

public interface IRoutingHandling {

    void writeMessage(Long key, HashSet<String> value);

}

public class Routing implements IRoutingHandling {

    private final Hashtable<Long, HashSet<String>> reverseLookup;

    private ASender asender;
    private BSender bsender;

    public Routing() {
        //Constructor work to be done here.. 
        reverseLookup = new Hashtable<Long, HashSet<String>>();
    }

    public void notify(TopicEvent event) {
        if (event.getMessage() instanceof AMessage) {
            asender = new ASender(this, event.getMessage())

        } else if (event.getMessage() instanceof BMessage) {
            bsender = new BSender(this, event.getMessage())

        }
    }

    @Override
    public void writeMessage(Long key, HashSet<String> value) {
        reverseLookup.put(key, value);
    }

}

public class ASender implements Runnable {

    private IRoutingHandling _routingHandling;

    public ASender(IRoutingHandling r, RoutingMessage rm) {
        _routingHandling = r;
        routingMessage = rm;
        this.run();
    }

    public void run() {
        handleMessage();
    }

    private void handleMessage() {
        // do some stuff and extract data from the routing message object

        _routingHandling.writeMessage(somethingToSetAsKey, somethingToSetAsValue)
    }

}

Thank you for your comments. I'll try to convert (is this the right term? I'm german..) your thoughts. — smsnheck, Jul 17 '13 at 10:49
One last question. Let's say i have a Vector. Since the Vector class is not thread safe, i have to synchronize on the add() and get() methodes, right? What is the better way? Synchronize via synchronized(vector) { vector.get(value); } Or public synchronized Vector getVector(String value){ return vector.get(value); } ? — smsnheck, Jul 17 '13 at 11:40
Actually, each of the methods of java.util.Vector is synchronized. See http://docs.oracle.com/javase/6/docs/api/java/util/Vector.html, so again, there's no need to add any synchronized keyword. — Velth, Jul 17 '13 at 11:56
Ok thank you. I had in my mind that the Vector isn't synchronized. — smsnheck, Jul 17 '13 at 12:22

Read and write into shared thread variables

1 Answers1