3

Below is my stripped down java code for review. I have several subclasses, and when execParallel() is called, a new thread is launched. This thread and the default thread must both execute criticalFunction() several times via actionFunction(), but this function will only work properly for a given SubClassC connection if executed by only one process at a time.

I have used the keyword "synchronized" to guard against concurrent executions, however in practice the criticalFunction() is in fact being called by both threads at the same time.

Any idea What I am doing wrong?

public class MainClass extends GlobalLibrary {
    public static SubClassA masterObj;

    public MainClass() {
        masterObj = new SubClassA();
    }        
    public static class SubClassA {
        public SubClassB subObj1;
        public SubClassB subObj2;        
        public SubClassA() {
            subObj1 = new SubClassB();
            subObj2 = new SubClassB();
        }
    }
    public static class SubClassB {
        public SubClassC conObj;        
        public Thread ut = null;        
        public SubClassB() {
            conObj = new SubClassC();
        }
    }
    public static class SubClassC {
        public TCPMasterConnection con=null;
        public SubClassC() {
            con = new TCPMasterConnection();
        }        
        public synchronized Object criticalFunction(int arg) {
            return otherClass.executeCritical(con, arg);
        }
    }    
    public boolean actionFunction(SubClassB subObj, int arg) {
        return (subObj.conObj.criticalFunction(arg)==null);
    }

    public class ActionThread implements Runnable {
        public SubClassB subObj;
        private int icode;
        public ActionThread(SubClassB arg1, int arg2) {
            subObj = arg1;
            icode = arg2;
        }
        public void run() {
            for (int i=0; i<10; i++) actionFunction(subObj, icode);
        }
    }    
    public void execParallel() {
        masterObj.subObj1.ut = new Thread(new ActionThread(masterObj.subObj1, 1));
        masterObj.subObj1.ut.start();

        actionFunction(masterObj.subObj1, 2);
        actionFunction(masterObj.subObj1, 3);
        actionFunction(masterObj.subObj1, 4);
        actionFunction(masterObj.subObj1, 5);
        actionFunction(masterObj.subObj1, 6);
    }
}
4

3 回答 3

1

If your goal is to protect the otherClass.executeCritical(con, arg) invocation, then you'll want to lock at the granularity of the otherClass instance. If the goal is to have only one thread using the "master connection" at a given time, which seems like it would be something you very much want, then you need the locking granularity to be at the instance of TCPMasterConnection. In the latter case, your code would look like this:

    public Object criticalFunction(int arg) {
        synchronized(con) {
          return otherClass.executeCritical(con, arg);
        }
    }

Now if you have multithread-unsafe code in both otherClass and con (of TCPMasterConnection), you'll maybe want a lock with larger granularity. In that case, an easy thing might be to lock at the class level as described in other answers.

于 2013-05-21T22:01:02.943 回答
0

Whether part of your code that you want to synchronize, make this instead:

Object lock = new Object();
public void doSomething{
   synchronized(lock){
    //your code
    }
}

Synchronized methods only work at the instance level.

于 2013-05-21T21:37:34.490 回答
0

You are calling criticalFunction() on different instances so they are using different locks. You need to share a lock between all instances.

Try this

public Object criticalFunction(int arg) {
    synchronized (SubClassC.class) {
        return otherClass.executeCritical(con, arg);
    }
}
于 2013-05-21T21:43:47.077 回答