Data commit issue in multithreading

Question

I am new to Java and Hibernate.

I have implemented a functionality where I generate request nos. based on already saved request no. This is done by finding the maximum request no. and incrementing it by 1,and then again save i it to database.

However I am facing issues with multithreading. When two threads access my code at the same time both generate same request no. My code is already synchronized. Please suggest some solution.

synchronized (this.getClass()) {
        System.out.println("start");

        certRequest.setRequestNbr(generateRequestNumber(certInsuranceRequestAddRq.getAccountInfo().getAccountNumberId()));
        reqId = Utils.getUniqueId();
        certRequest.setRequestId(reqId);
        ItemIdInfo itemIdInfo = new ItemIdInfo();
        itemIdInfo.setInsurerId(certRequest.getRequestId());
        certRequest.setItemIdInfo(itemIdInfo);
        dao.insert(certRequest);
        addAccountRel();

        System.out.println("end");
}

Following is the output showing my synchronization:

start
end
start
end

Is it some Hibernate issue. Does the use of transactional attribute in Spring affects the code commit in my Case?

I am using the following Transactional Attribute:

@Transactional(readOnly = false, propagation = Propagation.REQUIRED, rollbackFor = Exception.class)

EDIT: code for generateRequestNumber() shown in chat room.

    public String generateRequestNumber(String accNumber) throws Exception {
        String requestNumber = null;
        if (accNumber != null) {
            String SQL_QUERY = "select CERTREQUEST.requestNbr from CertRequest as CERTREQUEST, "
                    + "CertActObjRel as certActObjRel where certActObjRel.certificateObjkeyId=CERTREQUEST.requestId "
                    + " and certActObjRel.certObjTypeCd=:certObjTypeCd "
                    + " and certActObjRel.certAccountId=:accNumber ";

            String[] parameterNames = {"certObjTypeCd", "accNumber"};
            Object[] parameterVaues = new Object[]
                    {
                            Constants.REQUEST_RELATION_CODE, accNumber
                    };
            List<?> resultSet = dao.executeNamedQuery(SQL_QUERY,
                    parameterNames, parameterVaues);

// List<?> resultSet = dao.retrieveTableData(SQL_QUERY); 
            if (resultSet != null && resultSet.size() > 0) {
                requestNumber = (String) resultSet.get(0);
            }
            int maxRequestNumber = -1;
            if (requestNumber != null && requestNumber.length() > 0) {
                maxRequestNumber = maxValue(resultSet.toArray());
                requestNumber = Integer.toString(maxRequestNumber + 1);
            } else {
                requestNumber = Integer.toString(1);
            }
            System.out.println("inside function request number" + requestNumber);
            return requestNumber;
        }
        return null;
    }

Answer 1

First thing:

Why are you getting the thread inside the method. I is not required here.

Also, one thing;

Can you try like this once:

final static Object lock = new Object();

synchronized (lock)
{
 .....

}

what I feel is that object what you are calling is different so try this once.

Answer 2

You mentioned this information in your question.

I have implemented a functionality where I generate request nos. based        on already saved request no. This is done by finding the maximum request no. and incrementing it by 1,and then again save i it to database.

On a first look, it seems the problem caused by multi appserver code. Threads are synchronised inside one JVM(appserver). If you are using more than one appserver then you have to do it differently using more robust approach by using server to server communication or by batch allocation of request no to each appserver.

But, if you are using only one appserver and multiple threads accessing the same code then you can put a lock on the instance of the class rather then the class itself.

synchronized(this) { lastName = name; nameCount++; }

Or you can use the locks private to the class instance

private Object lock = new Object();
  .
  .
  synchronized(lock) {
         System.out.println("start");

    certRequest.setRequestNbr(generateRequestNumber(certInsuranceRequestAddRq.getAccountInfo().getAccountNumberId()));
    reqId = Utils.getUniqueId();
    certRequest.setRequestId(reqId);
    ItemIdInfo itemIdInfo = new ItemIdInfo();
    itemIdInfo.setInsurerId(certRequest.getRequestId());
    certRequest.setItemIdInfo(itemIdInfo);
    dao.insert(certRequest);
    addAccountRel();

    System.out.println("end");
    }

But make sure that your DB is updated by the new sequence no before the next thread is accessing it to get new one.

Answer 3

It is a good practice to generate "the request number (Unique Id)" by using the DATABASE SEQUENCE so that you don't need to synchronize your Service/DAO methods.

Answer 4

Don't synchronize on the Class instance obtained via getClass(). It can have some strange side effects. See https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=43647087

For example use:

synchronize(this) {
    // synchronized code
}

or

private synchronized void myMethod() {
    // synchronized code
}

To synchronize on the object instance.

Or do:

private static final Object lock = new Object();

private void myMethod() {
    synchronize(lock) {
        // synchronized code
    }
}

Like @diwakar suggested. This uses a constant field to synchronize on to guarantee that this code is synchronizing on the same lock.

EDIT: Based on information from chat, you are using a SELECT to get the maximum requestNumber and increasing the value in your code. Then this value is set on the CertRequest which is then persisted in the database via a DAO. If this persist action is not committed (e.g. by making the method @Transactional or some other means) then another thread will still see the old requestNumber value. So you could solve this by making the code transactional (how depends on which frameworks you use etc.). But I agree with @VA31's answer which states that you should use a database sequence for this instead of incrementing the value in code. Instead of a sequence you could also consider using an auto-incement field in CertRequest, something like:

@GeneratedValue(strategy=GenerationType.AUTO)
private int requestNumber;

For getting the next value from a sequence you can look at this question.