views:

720

answers:

1

I have a servlet that does some work for user and then decrement user's credit. When I watch user's credit in the database in real time, if there are many concurrent requests from the same user, the credit has been deducted incorrectly due to concurrency control. T Assume I have one server and database management used is hibernate. I am using transaction control to span the whole request, please see code for detail. I have several questions:
1. why are the credit counter in db jumping all over the place when facing many concurrent request from same user? why isn't my transaction control working?
2. if underlying data was modified after I retrieved user account and then attempt to update it, why didn't I get any HibernateException(eg.StaleObjectException)?
3. I have transaction span across the full user request, is there a better way? please critique. Feel free to rewrite the sample code structure if you feel I'm doing the whole thing wrong.

Main servlet class:
protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

      try{
       Manager.beginTransaction();
       cmdDowork(request, response);
       Manager.commitTransaction();
      }catch(Exception exp){
       Manager.rollbackTransaction();
       exp.printStackTrace();
      }
      finally{
       Manager.closeSession();
      }
}

public void cmdDowork(){
try{
     UserAccount userAccount = lazyGetUserAccount(request.getParameter("userName"));

     doWorkForUser(userAccount);//time and resource consuming process

     if(userAccount!=null) {

    decUserAccountQuota(userAccount);

     }

}catch (HibernateException e){
    e.printStackTrace();

}
}

public static UserAccount lazyGetUserAccount(String userName) {
     UserAccount userAccount = Manager.getUserAccount(userName);
     if(userAccount == null){
      userAccount = new UserAccount(userName);
      userAccount.setReserve(DEFAULT_USER_QUOTA);
      userAccount.setBalance(DEFAULT_USER_QUOTA);
      Manager.saveUserAccount(userAccount);
     }
     return userAccount;
}
    private boolean decUserAccountQuota(UserAccount userAccount) {

     if(userAccount.getBalance() 

Edit: code I used to test optimistic locking as suggested by the answer, I am not getting a any StaleObjectException, the update were committed successfully..

Session em1=Manager.sessionFactory.openSession(); Session em2=Manager.sessionFactory.openSession(); em1.getTransaction().begin(); em2.getTransaction().begin(); UserAccount c1 = (UserAccount)em1.get( UserAccount.class, "jonathan" ); UserAccount c2 = (UserAccount)em2.get( UserAccount.class, "jonathan" ); c1.setBalance( c1.getBalance() -1 ); em1.flush(); em1.getTransaction().commit(); System.out.println("balance1 is "+c2.getBalance()); c2.setBalance( c2.getBalance() -1 ); em2.flush(); // fail em2.getTransaction().commit(); System.out.println("balance2 is "+c2.getBalance());
+5  A: 

You have two ways to handle this situation: either with pessimist locking or with optimist locking. But you seem to use neither of both, which explain probably the incorrect behaviour.

  • With optimistic locking, Hibernate will check that the user account wasn't altered between the time it was read and saved. A concurrent transaction may then fail and be rolled back.

  • With pessimistic locking, you lock the row when you read it and it's unlocked only when transaction completes. This prevent a concurrent transaction to read data that would become stale.

Refreshing the entity may read new data or not depending whether the current transaction has already been committed or not, but is not a solution neither. Because you seem to also create the user account if it doesn't exist, you can't apply pessimist locking so easily. I would suggest you use optimistic locking then (and use for instance a timestamp to detect concurrent modifications).

Read this other question on SO about pessimist and optimist locking. Have also a look at hibernate chapter "transaction and concurrency" and "hibernate annotations".

It should be as simple as adding @Version on the corresponding field, the optimisticLockStrategy default value is VERSION (a separate column is used).

-- UPDATE --

You can test whether it works in a test case. I've created a simple entity Counter with an ID, value, and version fields.

 public class Counter implements Serializable {

    @Id
    @GeneratedValue(strategy=GenerationType.AUTO)
    @Basic(optional = false)
    @Column(name = "ID")
    private Integer id;

    @Column(name = "VALUE")
    private Integer value;

    @Column(name = "VERSION")
    @Version
    private Integer version;
    ...
}

If you update one entity sequentially it works:

  id = insertEntity( ... );

  em1.getTransaction().begin();               
  Counter c1 = em1.find( Counter.class, id );                
  c1.setValue( c1.getValue() + 1 );
  em1.flush();
  em1.getTransaction().commit();


  em2.getTransaction().begin();
  Counter c2 = em2.find( Counter.class, id );
  c2.setValue( c2.getValue() + 1 );
  em2.flush(); // OK
  em2.getTransaction().commit();

I get one entity with value=2 and version=2.

If I simulate two concurrent updates:

id = insertEntity( ... );

em1.getTransaction().begin();
em2.getTransaction().begin();

Counter c1 = em1.find( Counter.class, id );
Counter c2 = em2.find( Counter.class, id );

c1.setValue( c1.getValue() + 1 );
em1.flush();    
em1.getTransaction().commit();

c2.setValue( c2.getValue() + 1 );
em2.flush(); // fail    
em2.getTransaction().commit();

then the 2nd flush fails:

Hibernate: update COUNTER set VALUE=?, VERSION=? where ID=? and VERSION=?
Hibernate: update COUNTER set VALUE=?, VERSION=? where ID=? and VERSION=?
Dec 23, 2009 11:08:46 AM org.hibernate.event.def.AbstractFlushingEventListener performExecutions
SEVERE: Could not synchronize database state with session
org.hibernate.StaleObjectStateException: Row was updated or deleted by another transaction (or unsaved-value mapping was incorrect): [org.ewe.Counter#15]
        at org.hibernate.persister.entity.AbstractEntityPersister.check(AbstractEntityPersister.java:1765)

This is so because the actual parameters in the SQL statements are:

   update COUNTER set VALUE=1, VERSION=1 where ID=xxx and VERSION=0   
   --> 1 row updated
   update COUNTER set VALUE=1, VERSION=1 where ID=xxx and VERSION=0   
   --> 0 row updated, because version has been changed in between
ewernli
great answer, to use optimistic locking, so I simply create an extra field in the table and annotate it with @version? would hibernate generate the version number automatically?
That's the idea, yes. "The application must not alter the version number set up by Hibernate in any way" from the link to hib. annotations.
ewernli
It still doesnt work, the versions never get to be greater than 1 even when there are many many concurrent requests. and the user account credit is still jumping all over the place.
Try to write a test case to see what goes wrong. The version number should be incremented if you update the same entity sequentially.
ewernli
I tried your test, but I am instead getting a locked timeout exception when calling em2.flush(), please see this question for detail: http://stackoverflow.com/questions/1956950/hibernate-lock-wait-timeout-exceeded
Just move the 1st commit after the 1st flush, and the 2nd commit after the 2nd flush. Sorry for the mistake, I did the test with MySQL and an MyISAM table, which are not transacted so the test worked. I switched to InnoDB table and yes I get a lock timeout. I hope it's ok now.
ewernli
strangely, both commits are successful without getting StaleObjectException.
I suspect that transaction is not working for my hibernate environment, how do I check?
INFO TransactionFactoryFactory:59 - Using default transaction strategy (direct JDBC transactions)INFO TransactionManagerLookupFactory:80 - No TransactionManagerLookup configured (in JTA environment, use of read-write or transactional second-level cache is not recommended)in hibernate startup log, what does this mean?
Transactions are working, otherwise you wouldn't have got a lock timeout the first time.
ewernli
The log says that Hib. will use plain JDBC transactions not JTA transactions (JTA is used in app. server mostly and support distributed transactions). This is ok.
ewernli
Please turn on show sql and check if the SQL in the log match what I've posted. The SQL where clause should have two parts: <ID>=? AND <VERSION_COLUMN>=?. How did you configure the mapping of your entity?
ewernli
both update SQL do show ID and version in the where clauses. I put a debugging statement in getVersion() and setVersion() it always print 1.
problem solved, I had problem solved, I had <version name="version" type="java.lang.Integer" generated="always" column="version"/> the generated was not letting hibernate update version
now how do I use optimistic locking to achieve the effect I want: properly decrement the balance by one for every transaction.
maybe I should use pessimistic locking?
Glad to hear that you are able to reproduce StaleObjectStateException and that the version number is incremented automatically.
ewernli
With optimistic locking, if there are two concurrent transactions, one transaction will suceed and decrement the balance, the other will fail. The balance will be decreased consistently. Isn't it ok? What are the chances in real that two concurrent requests for the SAME account are processed at the SAME time? Probably low I guess...
ewernli
You can try to send a lot of request, say 200, on the servlet for the same account. Several requests will be rolled back due to the StaleObjectException, but the one that will succeed will decrement the balance correctly.
ewernli
Sorry I forgot to mention that I get many concurrent transactions coming from the same account(a few per seconds), so the chances of failure using optimistic locking is too great(say I have 200 transactions, only 1/4 is counted at the end as a result of failure to update balance), I resorted to pessimistic locking. Is there a better way?
Pessimist locking will be hard because you create the account if it doesn't already exist (but it can still be done: one tx create the account if it doesn't exist, and a 2nd tx locks it and decrement the balance). Did you consider changing the client so that only one request is sent with the amount to decrement (instead of 1 each time)? Did you consider using the synchronized keyword to serialize all calls to the servlet?
ewernli
changing the client with amount to decrement does not work for my business case, synchronized keyword to serialize all calls to servlet would be too inefficient because I only need synchronization for each client account. I am pessimistic locking and have a single transaction spanning the whole user request. I will make two transactions as you suggested. Transaction itself doesn't lock db in any way right? it only serves as the boundary for rollback and commit, is this right? is there a limit as to how many transactions I can have concurrently?