Option a/ sounds easiest - but how much time does the cache save? (Enough to make it worthwhile working on? I'm guessing 'yes'. )
Option b/ sounds like it would lose all the benefits of the cache unless it's used in a tight loop somewhere. Option c/ sounds workable - is the query/tree easy to clone? (I'm guessing it's not trivial) Is there an option d/ for making the ParameterTranslationImpl threadsafe? (If I/we could get rid of the call to AdjustNamedParameterLocationsForQueryParameters() would it be threadsafe again?) From: Ayende Rahien Sent: Thursday, September 10, 2009 7:27 PM To: nhibernate-development Subject: [nhibernate-development] Re: Thread Safety issue with NH 2.1.x Okay, correction, I am not sure how to fix this issue. ParameterTranslationImpl must be stateful for the lifetime of the query. The problem is that we must never give the same query to two threads at the same time. This basically kills our query plan cache purpose. As I see it, we have three options: a/ disable the query cache completely b/ make the query cache a per thread query c/ clone the query from the cache when we give it out Thoughts? On Thu, Sep 10, 2009 at 8:58 PM, Ayende Rahien <[email protected]> wrote: I am following on a bug report that I got, and I have confirmed that we have a thread safety issue within NHibernate. The conditions for that are quite esoteric, which is why no one got it so far. I attached a patch the expose the issue. This was introduced in r4679, to fix NH-1908. The problem is that in ParameterTranslationImpl, we get the wrong value. There is a note there that says: // Steve Strong Note: The original Java does not do this decrement; it increments i for // every parameter type. However, within the Loader.GetParameterTypes() method, this introduces // nulls into the paramTypeList array, which in turn causes Loader.ConvertITypesToSqlTypes() to crash // with a null dereference. An alternative fix is to change the Loader to handle the null. I'm // not sure which fix is the most appropriate. // Legacy.FumTest.CompositeIDQuery() shows the bug if you remove the decrement below... That is indeed the error that I am getting most of the time, but that isn't what is bother me. What is bother me is that we have a test that fails only under multi threaded conditions. I tracked it down and it looks like the issue is with the query plan cache. It will serve the same instance of a query plan for several threads at the same time. That is fine on its own, the problem is that there are things there, like ParameterTranslationImpl, which are stateful, and get corrupted when used from multiple threads. I will commit a fix for this particular issue shortly, but it is something that we have to consider as well, because I don't think that we want to take the burden of ensuring that everything works right all the time under multi threading scenarios.
<<Emoticon10.gif>>
