Concurrency issues
Status: Beta
Brought to you by:
hibbert
Menu
▾
▴
#12 Concurrency issues
Before I start with this bug report I'd like to say a big thank you to you for creating Zocalo. It truly is an amazing piece of work. I've been reading quite a bit about PMs lately and it just wouldn't have been the same if I didn't have software to play with to ease the learning curve. Zocalo provides just that, and it does a great job. So once again, many thanks.
Now on to the bug report. There's a concurrency issue with the static HibernateUtil.currentSession() method. In a heavily used Zocalo system, the static currentSession() method could be invoked by a second web request before the first request has managed to close the session (via HibernateUtil.closeSession()). Since sessions are held on to in the static sessionHolder there is a realistic potential that 2 different web requests could end up referencing the same session. This will in turn lead to all sorts of data corruption issues.
Actually, looking deeper into this it seems that in most cases HibernateUtil.closeSession() is never even called. Following the Markets.jsp lifecycle reveals an example of this. It looks like ReloadablePage.commitTransaction() should be calling HibernateUtil.closeSession() instead of HibernateUtil.currentSession().close(). Without HibernateUtil.closeSession() the result is a single session object shared by all incoming web requests, including concurrent ones.
I have a suggestion for how to resolve this. It involves using Hibernate's getCurrentSession() API instead of openSession(). This way there's no need to worry about having shared session objects because Hibernate will guarantee a unique session for each thread.
I'll come up with some code fix proposals tomorrow. Right now I need some sleep. If you go for this getCurrentSession() fix it will also require some configuration changes in hibernate.properties to get it to work correctly. I'll include this in my proposed fixes tomorrow.
Thanks for the kind words.
I don't take offense at the suggestion. I recently found (and thought I had addressed) another concurrency bug, so it's not the first one.
Before you get too far into the solution, I think the concurrency problem may be addressed by the HandlerWrapper in SerletUtil::initializeServerTopLevel(). The intent is that that wrapper puts a synchronized block around every access to the server, so different threads can't interfere. I've been thinking that I should find a way to ensure that only the threads that might access the market engine (those called from the Jsp pages) should actually get wrapped this way. Access to .css, .js, and charts shouldn't need to be synchronized. If there's a cleaner way to prevent interactions among requests, I'd be glad to hear about it.
I still need to take a look at the session close code to see what I am doing wrong there.
Thanks for pointing out these problems. I definitely appreciate it.
I think you're right about closeSession() vs. currentSession().close(). I'll make that change before the release I'm hoping to publish this week.
And I'm not convinced that there's any need to worry about parallel threads. After I discovered a previous concurrency bug, I concluded that the market code in Zocalo isn't reentrant, so preventing concurrency was the right approach. I think the HandlerWrapper in ServerUtil achieves this. (It certainly eliminated the evidence I was seeing previously that multiple transactions were getting into the trading code simultaneously. This was happening with 15 to 20 active, simultaneous traders.) Please let me know if you see other indications about concurrency or reentrant code.
Making it re-entrant yet thread-safe was the whole point of my bringing this up. I think it would be terrible to turn the web app into a single-thread system. It's like running the web app on an old MSDOS operating system.
I think this will become clearer when I send you my code fixes. Just gimme a few more minutes...
Sorry for the delay, this took awhile because I had to move the existing Hibernate3.jar up to version 3.3.5 to get the nice thread-bound getCurrentSession() features, which were introduced in version 3.1 . I've attached my copy of the devel folder which contains the changes I made to make requests thread-safe yet concurrent. The meat of the change is in ReloadablePage.java's beginTransaction() and commitTransaction(). I modified HibernateUtil's currentSession() to do exactly that, return the current session context instead of opening a new session. This current session context is managed by Hibernate. It closes and flushes automatically upon commit() or rollback(), you don't have to call session.close() on it. You can call getCurrentSession() as many times during a transaction and Hibernate will return the same session context, kind of like what you did with the sessionHolder variable but in a manner that guarantees thread safety. So you don't have to worry when other methods or other threads call getCurrentSession() during an already running transaction.
The other way to fix this is to stay with the current way of opening and closing sessions but to bind each session to the requesting thread rather than to a static sessionHolder variable. I didn't choose this route because I think Session.getCurrentSession() is a more elegant approach that already guarantees thread-safety without the hassle of refactoring the code to remove the sessionHolder static variable, etc.
So in short, since the web container creates a new thread for each incoming web request what you have now is a concurrent thread-safe Hibernate session management strategy. Let me know what you think.
Am having trouble attaching the file because SF won't let me attach anything more than 256k . Urgh.
Is there any other way I can get the file to you?
I'm surprised that concurrency in the db layer will allow enough transactions to interleave that it will be a win. I doubt that I have specified very fine-grained transactions, so I'd expect the granularity of the atomicity to be at the level of individual web requests anyway. Is it your experience that there's sufficient parallelism in ordinary well-written OO code?
Even if transactions run in separate threads, they still rely on access to the same underlying objects. Whether Hibernate uses optimistic or pessimistic concurrency, I'd expect the transactions to have to be interleaved at the level of web requests. Why will this be able to allow more concurrency? Book orders could be satisfied out of order, but everything that happens with a marketmaker has to be serialized to make any sense at all. (That was the evidence for the previous concurrency bug; marketmaker prices were changing in ways that indicated that they weren't serializing properly.)
I will send you info at your sourceforge address on a location where you can drop off software for me. (portend.org)
I've uploaded the file. I have a feeling the issue I describe is exactly what is causing the marketmaker synchronization issues you were having. Session and Transaction objects were shared at the static level which means separate web requests could override each other. I don't know the specifics of the issue but my guess is that it is either this or that the db transaction level needs to be increased.
DBs do more than atomizing all transactions. If you stuff every web request into a single thread you lose the ability to have the DB allow multiple simultaneous reads, which is ok and great for performance. The DB only starts to work harder when writes are involved. One can leverage the db's transaction management capabilities if the OO code is thread-safe. As long as each incoming web request is allocated its own stack (which is what the web container does) and as long as you're not sharing object instances meant for a single request across multiple requests then we're golden.
This is definitely doable (and I will even suggest that it is the norm in any web application). I would be out of a job if web applications were not able to handle large number of requests concurrently. Check out some literature on the servlet's SingleThreadModel, which is what you are achieving with the HandlerWrapper, and why it is not recommended by Sun. I can't go into more detail now as I have to leave for work.
Also note that the code I uploaded is not ready to be committed into SVN. There are other issues I need to bring up with you which I will do when I get back today.
Please don't take my comments as offense. I am sharing all this with you because I love Zocalo and want to see it improve.
Hope you had a chance to review the changes. One thing I didn't get a chance to note earlier was that the current version of Hibernate I put in place of the older 3.0 one requires all query generating operations to be demarcated in a transaction. So in createMarkets.jsp for example, this will no longer work when creating a new market:
<jsp:useBean id="createMarkets" scope="session" class="net.commerce.zocalo.JspSupport.MarketCreation" />
<jsp:setProperty name="createMarkets" property="*" />
because setProperty on the userName property will eventually call a Hibernate operation (createCriteria() in this case). And since the session/transaction hasn't begun, an exception is thrown by Hibernate. The solution? Start the transaction right before you are going to perform any query related operation in Hibernate:
<jsp:useBean id="createMarkets" scope="session" class="net.commerce.zocalo.JspSupport.MarketCreation" />
<% createMarkets.beginTransaction(); %>
<jsp:setProperty name="createMarkets" property="*" />
createMarkets.jsp isn't the only place I found this exception to occur, so you'll have to go in and change every occurence of this. You are probably thinking to yourself right now: Is this really worth the effort just to be able to process requests concurrently? My answer is a resounding yes. Without concurrent processing of requests the web would not be what it is today. Even the very earliest versions of web servers could handle concurrent processing.
I got the file, thanks. I've looked through the changes, and it appears that the important changes are in the new version of hibernate. I've searched the hibernate site, and haven't found any description of the changes you are relying on. Do you know where I can find anything that describes those improvements?
Even before those changes, hibernate shouldn't have been allowing queries outside of transactions. I didn't realize it was allowing that, or I would have looked harder for cases like the one you point to. That needs to be fixed independent of whether we make the change you're recommending. I had assumed Hibernate would warn me of that kind of thing.
I was originally thinking of concurrency in terms of a single market at a time. It would be a significant mistake for Zocalo to allow more than one transaction (especially those relying on a market maker) to overlap in going through the central trading algorithms. But then I remembered that Zocalo supports multiple markets, (doh!) and there's no reason to exclude simultaneous operations on separate markets. I think it may make sense to add synchronize blocks around access to the auctioneer even after making this change to enable additional concurrency.
I don't remember where I originally read it, was quite a while ago. But I found some mentions here:
http://www.hibernate.org/hib_docs/v3/reference/en-US/html/tutorial-firstapp.html
http://www.hibernate.org/hib_docs/v3/reference/en-US/html/transactions.html
One other thing I noticed from the recent changes you committed, closeSession() doesn't need to be used with getCurrentSession().