Java: Fail in synchronizing threads

Question

I have the following code:

for (int iThreadCounter = 1; iThreadCounter <= CONNECTIONS_NUM; iThreadCounter++){ 
  WorkThread wt = new WorkThread(iThreadCounter);
  new Thread(wt).start(); 
  m_arrWorkThreadsToCreate.add(wt); 
} 

Those threads calls the following code:

int res = m_spLegJoin.call(m_workTread, m_workTread.getConfId());

And this is the call method inside LegJoinSp class:

public class LegJoinSp extends ConnEventSp {

    private static final int _LEG_JOIN_ACTION_CODE = 22;
    private static int m_nLegId = Integer.valueOf(IniUtils.getIniValue("General", "LEG_ID_START"));
    private final Lock m_lock = new ReentrantLock();

    public int call(WorkThread a_workThread, String a_sConfId) {

        synchronized (this) {

            //m_lock.lock();
            m_nLegId++;

            boolean bPass = false;

            Log4jWrapper.writeLog(LogLevelEnum.DEBUG, "LegJoinSp - call", "a_workThread = " + a_workThread.getThreadId() + " a_sConfId = " + a_sConfId);

            if (super.call(a_workThread, a_sConfId, _LEG_JOIN_ACTION_CODE, "" + m_nLegId) == 0) {

                bPass = true;

            } else {
                bPass = false;
            }

            //m_lock.unlock();

            if (bPass) {

                Log4jWrapper.writeLog(LogLevelEnum.DEBUG, "LegJoinSp - call", "a_workThread = " + a_workThread.getThreadId() + " a_sConfId = " + a_sConfId + " returned leg id " + m_nLegId);

                return m_nLegId;
            } else {

                return -1;
            }
        }

    }

    public Lock getLock() {
        return m_lock;
    }

}

I've got 2 threads calling this call() method. m_nLegId is initiated with 100. As you can see I have tried to lock the method with both

synchronized(this)

and

m_lock.lock() and m_lock.unlock()

The problem is that when I first get to if (bPass) inner code, it write 102 to my log as the m_nLegId value. However I expect it to be 101 because of the m_nLegId++; statement. It seems that the second thread manage to get inside the code before the synchronize block ends for the first thread execution.

How can I fix that?

Thank you

Answer 1

For me your bug is related to the fact that m_nLegId is a static field and you try to synchronize access on the current instance instead of the class such that you don't properly prevent concurrent modifications of your field.

I mean

synchronized (this) {

Should rather be

synchronized (LegJoinSp.class) {

NB: In case you only need a counter, consider using an AtomicInteger for your field instead of an int.