NH-2500 - NH 3.0 Linq provider uses query parameters from first call in subsequent calls.

[nhibernate-bot]

Andrey Titov created an issue — 13th January 2011, 10:32:53:

NH 3.0 Linq provider uses query parameters from first call in subsequent calls if this parameters are not propagated to SQL.

//in first session
this.number = 545;
var cats = session.Query<Cat>().Select(cat => new CatInfo { Name = cat.Name, SomeNumber = this.number }).ToList();
// all works perfect: all CatInfos contains 545

//later in another session
this.number = 842;
var cats = session.Query<Cat>().Select(cat => new CatInfo { Name = cat.Name, SomeNumber = this.number }).ToList();
// magic happens: all CatInfos contains 545 again

I've debugged NH and found that in second query it gets QueryPlan from cache in NHibernate.Impl.AbstractSessionImpl.GetHQLQueryPlan() and it contains parameters from first call of query (NHibernate.Linq.NhLinqExpression.ParameterValuesByName) and then fails to replace them in NHibernate.Linq.NhQueryProvider.SetParameters() because they are not actual SQL parameters (query.NamedParameters).

Sample solution is attached (VS2010, SQL Server Express DB). Put NH and LinFu.DynamicProxy binaries to .Libraries\NHibernate before build.

I think actual parameters should be cleared from query (set to default(T)) before putting plan in QueryPlanCache to let them be garbage collected and prevent real magic happen in case of similar bugs.

---

Nicola Tuveri added a comment — 23rd January 2011, 10:06:28:

Attached a patch that fix the issue. The patch is generated targeting revision 5365.

Constants were compiled into the lambda used to shape the result.
In the patch constants are converted into parameters and refreshed each execution of a cached query.
The issue is the same demonstrated in the test attached to NH-2397 by Joao Braganca.

---

Patrick Earl added a comment — 23rd January 2011, 21:22:05:

Hi Nicola. I looked through your patch and have a couple questions.

1. Have you considered what might happen with subqueries? I'm still learning about the code myself, but I saw some indication that subqueries also use the result transformer, and I'm not sure if the current code covers those cases. I came up with this thought after looking to see if the ResultTransformer constructor could be called with the list of constants, instead of setting it directly as a property.

2. It seems like the ExpressionKeyVisitor would also need to be adjusted. Did you or could you look into this as well?

3. As a random thought, it would be good if NHibernate didn't maintain references to the external objects in any query/expression cache so things can be garbage collected properly.

Thanks so much for the work you put into the patch!

---

Nicola Tuveri added a comment — 24th January 2011, 2:36:16:

Hi Patrick.

1. Good point. I'll write some tests to cover more complex queries, starting with subqueries.
   Using a property to set the list of constants like I did it's dirty, I know.
   On the other hand the constructor of ResultTransformer is called before the query is cached, so constants would be cached as well.
   I'll try to find a better way to accomplish this, checking other classes as well.
   Could ExpressionQueryImpl and HQLExpressionQueryPlan along with thier related interfaces be good candidates?

I'll check how NHibernate handles constants in regular (non Linq) queries too.

2. Yes I did, but I don't understand the exact point you are referring to.
   I would be happy to adjust ExpressionKeyVisitor following your suggestion.

3. Right. Are you referring to the Constants property of the result transformer?

Thanks for reviewing the patch!

---

Andrey Titov added a comment — 24th January 2011, 12:47:39:

Please be aware in NHibernate-3.0.0.GA plans are cached by string key not containing types. So second queries in following pairs will throw InvalidCastException:

var cats = session.Query().Select(cat => new CatInfo { Name = cat.Name, SomeObject = "8989", }).ToList();
var cats = session.Query().Select(cat => new CatInfo { Name = cat.Name, SomeObject = 5675, }).ToList();

const string number = "3876";
var cats = session.Query().Select(cat => number).ToList();
const int number = 4378;
var cats = session.Query().Select(cat => number).ToList();

Two similar bugs were fixed: NH-2433 and NH-2459. I havn't check does this fix covers this scenario, but please take care to types when substityting constant values.

---

Cameron Harris added a comment — 10th May 2011, 2:57:11:

I've created an example for what I believe to be this bug (or at least related). It happens in every version of NHibernate I have tested, from NH3.0GA through to NH3.2Alpha3.

The bug occurs if you pass it a projection expression with the same structure twice, where the projection is a method on an object. The second time you call it, it will execute the method on the first object you passed in, which it appears to have cached. This incorrect behaviour appears to persist for the whole lifetime of the session factory, not just the current session or transaction. Hanging onto user objects could be a bit memory wasteful too..

var foos1 = s.Query().Select(x => selector1.Map(x));
var foos2 = s.Query().Select(x => selector2.Map(x)); // does the same as foos1
var foos3 = s.Query().Select(selector2.Map); // works fine

= Expected output =
S1:Test 1; S1:Test 2
S2:Test 1; S2:Test 2
S2:Test 1; S2:Test 2

= Actual output =
S1:Test 1; S1:Test 2
S1:Test 1; S1:Test 2
S2:Test 1; S2:Test 2

---

Cameron Harris added a comment — 10th May 2011, 6:03:33:

Attached a test-case as per welcome message. Does the same as my test program above.

---

Greg Roberts added a comment — 5th January 2012, 17:28:33:

Has there been any movement on this issue? We found this recently and renders using linq somewhat worthless if there are hard to debug traps like this...

---

Pavel Savara added a comment — 22nd February 2012, 7:00:38:

Maybe Expression.Quote could help ?
http://msmvps.com/blogs/jon_skeet/archive/2011/02/20/reimplementing-linq-to-objects-part-43-out-of-process-queries-with-iqueryable.aspx
http://stackoverflow.com/questions/3716492/what-does-expression-quote-do-that-expression-constant-cant-already-do

---

Farzad Panahi added a comment — 15th September 2012, 2:08:49:

Is NHLQ-58 related to this issue?

---

Daniel Laberge added a comment — 25th February 2014, 22:23:51:

If this won't be fixed, could we think about detecting such cases and throwing an exception so developers don't introduce non-obvious bugs in their queries? What do you guys think?

In our project we've been burned 4 times in the past two years with this bug (of which I've just now confirmed the root cause by finding this issue). All of which made it into production (QA didn't detect them because the query works the first time, hehe).

---

Alexander Zaytsev added a comment — 27th February 2014, 2:21:49:

<~dlaberge>, if we can identify that situations then we can fix them. The problem is that we unable to detect all possible situations.

---

Scott Whitlock added a comment — 27th February 2014, 11:59:59:

I've been burned by this several times. It's completely unintuitive until you really grasp what it's doing under the hood, and even then it's hard to avoid. In order to always avoid this issue, now I always just return full entities from my queries, and then I translate into DTO's, but that completely nullifies any performance advantage I would be seeing from the cache feature that's creating this problem. How about removing the query cache feature completely until it can be done correctly?

---

catahoc added a comment — 17th September 2014, 9:47:09:

Hey, guys, what about implementing another feature - disable cache (permanently or temporarely).
We already did this by reflection (see an attachment 'QueryPlanCacheControl.cs') in our project. Through Reflection we are receiving QyerPlanCache instance and replace its fields with empty mocks. So, cache always misses, but query executes correctly.
We didn't see big performance issues in our solution, so we disabled cache permanently. If you think, that performance reducing, so you can replace caching values back, after "dangerous" query completes execution.

---

Ricardo Peres added a comment — 18th September 2014, 9:08:41:

<~catahoc>: I have submitted a similar suggestion: NH-3231.
Haven't given up on that, but it would work per query. For permanent solutions, use a stateless session.

---

Frédéric Delaporte added a comment — 13th July 2017, 17:12:35:

NH-3231 is unrelated, it is about the first level entity cache. Here the trouble lies in the query plan cache. This does not cache anything entity related.

It appears a (ab3c45bec4c45187b4243278d0439aba700207dc] and a [test|https://github.com/nhibernate/nhibernate-core/commit/1097114d87e088be8d17acc658eebeddff90f67d) have been contributed in January 2012, without reporting here. They have made it in 3.3.0 version. But I suspect they have fixed only a subset of the trouble.

More tests should be contributed and checked/fixed. The fix looks a bit hackish to me, but I have not analyzed it thoroughly.

[fredericDelaporte]

Most tests join on the Jira ticket are now green thanks to ab3c45b (tests not yet merged, I may PR them).
Only the case of the captured object instance from opener's tests is still failing. Another case not tested in those NH-2500 test is also still failing, see #1330.

ab3c45b change can be slightly amended to support #1330 case too. (By setting the insideSelectClause member only if method declaring type namespace is System.Linq.)

About the captured object instance case, I currently only see a workaround: override its ToString() to yield an unique value per instance. Thanks to ab3c45b (update: further patched for supporting #1330), it will be included in the query plan cache key, and so it will avoid re-using a previous different instance in a subsequent query.

Still, ab3c45b looks quite hackish to me. The patch proposed by Nicola Tuveri should be investigated as an alternate, maybe better fix. Ideally, it should be completed by a detection of object instance caching cases, where query plan caching should then be disabled for avoiding retaining these object instances.

Update: there is also a proposed patch for these constants trouble in #866 which should be investigated.