0

I have a java servlet which interacts with hibernate . It is necessary to generate a check id on the system thus: 

for (long j = 0; j < soldProductQuantity.longValue(); j++) {
    checkId = Hotel.getNextMcsCheckAndIncrement();                  
    checkIdString = checkId.toString();
    McsCheck.generateCheck(checkIdString);                                  
}

where getNextMcsCheckAndIncrement() is defined as 

 static public synchronized Long getNextMcsCheckAndIncrement()

It pulls a value from the database using hibernate, does some operations on it, stores the modified value back, then returns the number.

Because getNextMcsCheckAndIncrement is synchronized, I would expect no two checks to have the same number, because no two threads could enter that method at the same time.

Yet I can see in my data repeated instances of multiple check ids. So clearly this isn't working. What am I missing?

The implementation of getNext as asked: 

// Increment FIRST, then return the resulting value as the current MCS check value.
static public synchronized Long getNextMcsCheckAndIncrement() {
    Hotel theHotel = null; 
    Long checkCounter; 
    @SuppressWarnings("unchecked")                  
    List<Hotel> hotelList = Hotel.returnAllObjects(); 
    for (Hotel currentHotel : hotelList) {  // there should be only one. 
                theHotel = currentHotel;
        }

    checkCounter = theHotel.getMcsCheckCounter()+1; 
    theHotel.setMcsCheckCounter(checkCounter);
    theHotel.update(); 
    return checkCounter; 

} 

static public List returnAllObjects() {
    return Hotel.query ("from Hotel"); 
}   

static public List query(String queryString) {
    Session session = HibernateUtil.getSessionFactory().openSession();
    List result = session.createQuery(queryString).list();  
    session.close(); 
    return result;
}

public void update() {
    Session session = HibernateUtil.getSessionFactory().openSession();
    Transaction transaction = session.beginTransaction();
    session.update(this);
    transaction.commit();
    session.close(); 
}

Yes, I know it's not the best way to do it, and I'll solve that in time. But the immediate issue is why concurrency fails.

bpendell
  • 1
  • 2
  • 1
    (You can change your profile to add your name. Don't add thank-yous and signatures in questions, it is noise.) – Sotirios Delimanolis May 13 '14 at 22:37
  • @user3634440 Edit your post instead of placing code in comments. – ug_ May 13 '14 at 22:43
  • Noted. Just solved that and will delete the offending comment. The post has been updated. – bpendell May 13 '14 at 22:45
  • Can you show us the `returnAllObjects` method? – Dawood ibn Kareem May 13 '14 at 22:47
  • David Wallace: Yes, just updated. – bpendell May 13 '14 at 22:50
  • I suspect the hotel list returned is not the same order on every invoke. In addition, it is always taking the last element in the list and doing calculation based on that hotel. I suspect the last element of the list is different. – anonymous May 13 '14 at 22:53
  • in any case, I've made a first attempt at solving the problem by putting the entire block above into a synchronization block. So the entire FOR loop is protected rather than just the call to getNext . I'd feel a lot better about it, however, if I knew why the synchronization had broken in the first place. – bpendell May 13 '14 at 22:55
  • "I suspect the hotel list returned is not the same order on every invoke. In addition, it is always taking the last element in the list and doing calculation based on that hotel. I suspect the last element of the list is different." Unlikely. There is only ever one element in the list. I have confirmed that in this particular instance. – bpendell May 13 '14 at 22:56
  • OK, the other thing to check is the call to `update()`. Does that actually `commits` into the DB? – anonymous May 13 '14 at 22:59
  • anonymous -- yes, it does. Just uploaded the code into the main question. As you can see, update opens a transaction, executes the command, commits, and closes the transaction out. A completely atomic operation. – bpendell May 13 '14 at 23:04
  • Ok, one final idea from me to try. Immediately after call to `update`, make a call to `returnAllObjects` and check the newly return list with a single instance of Hotel has the new value. – anonymous May 13 '14 at 23:19
  • ... How does that help? I can't reproduce this on the fly -- it's something that crops up over time. Something I'm tempted to do is put the value in a static variable and , instead of pulling it from the database every time, to pull it only the first time and after that read the static variable, even though I'll still continue to write the updates. That way I wouldn't be dependent on a hibernate call every time I need the value -- the value returned would always be in memory, and perhaps better protected by synchronization. – bpendell May 13 '14 at 23:33
  • Aha.. `something the crops up over time`. Does that mean it only occurs under load? Do you have multiple servlets? Are there any other code besides `getNextMcsCheckAndIncrement()` that load and save to the Hotel table? If so, you are getting synchronization issues with other code that loads/saves into the DB. I am not well verse with Hibernate, so the last test I proposed is merely to see whether `query` would give you back the new value after you call `update`. Good luck. – anonymous May 14 '14 at 00:39
  • "Does that mean it only occurs under load? Do you have multiple servlets? Are there any other code besides getNextMcsCheckAndIncrement() that load and save to the Hotel table?" Yes, Yes, and Yes. "If so, you are getting synchronization issues with other code that loads/saves into the DB" I believe this is the answer. Feel free to make it the official answer. – bpendell May 14 '14 at 12:48

1 Answers1

0

Anonymous in comments gave the correct answer: The problem must be the Hotel object in the hibernate database, not the synchronization method. Although the counter method is correctly synchronized, if the hotel object is being modified outside of this algorithm, those accesses will NOT be synchronized.

The correct answer, therefore, is to closely check all database accesses to the hotel object and ensure that the object is not modified elsewhere when this loop is in progress, or to refactor the counter out of the Hotel object into dedicated storage.

bpendell
  • 1
  • 2