Session leak in NHLinq Query Plan Cache

[Lauri Kotilainen]

Hi,

I originally posted the description of this issue to the nhusers list:

https://groups.google.com/d/topic/nhusers/v_6WCod79XE/discussion

and I won't waste bits by pasting the entire description here unless it's deemed necessary. Anyway, I think I have a patch that fixes the session leak, but I don't understand the big picture well enough to evaluate whether or not it's a safe change. Essentially, what I did was move most of the code from NhLinqExpression.Translate to the NhLinqExpression constructor and eliminated the _expression field, making it a local variable in the ctor. It didn't cause any test failures in the master branch, and after I backported it to the 3.4.x branch and tested with the problematic application, the leak is gone (according to ANTS profiler).

Here's the patch:

https://gist.github.com/rytmis/3735cfc274e135aae753

Unfortunately, even with that patch applied, the unit test in my other post fails -- even though a heap inspection with WinDBG confirms that the session is no longer rooted and is eligible for collection.

What I would like to know at this point is whether the change is likely to cause performance regressions or other unexpected side effects.

Thanks,

-Lauri

[Gunnar Liljas]

Great work, Lauri!

I'll do some tests tomorrow, just to give you feedback.

/G

2013/12/11 Lauri Kotilainen <ryt...@gmail.com>

--
 
---
You received this message because you are subscribed to the Google Groups "nhibernate-development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to nhibernate-develo...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[Gunnar Liljas]

So far it looks OK, although it's a bit troubling that the query plan cache does this. I'm not a big fan of heavy constructors, but in this case it makes some sense. 

I think you can make your test behave correctly (i.e fail when it should) by adding a second pass of GC.Collect()

A JIRA issue and an accompanying unit test (for now, remove the dependency on SQLite) would be nice.

/G

2013/12/12 Gunnar Liljas <gunnar...@gmail.com>

[Lauri Kotilainen]

Which part is troubling? The part about the leak or the part about having the query plan cache end up constructing the expression?

If it's the latter, I did try other approaches too. I don't really like heavy ctors either, but the QPC and NhLinqExpressions are used in a variety of ways, and all the alternatives I could think of ended up causing breakage all over the place. This might be due to the fact that I don't have a very good mental map of the dependencies yet, though.

I'll file an issue soon-ish with the changes you proposed to the test. 

-Lauri

2013/12/12 Gunnar Liljas <gunnar...@gmail.com>

2013/12/11 Lauri Kotilainen <ryt...@gmail.com>

To unsubscribe from this group and stop receiving emails from it, send an email to nhibernate-development+unsub...@googlegroups.com.

[Gunnar Liljas]

It's troubling that the leak occurred in the first place.

/G

2013/12/15 Lauri Kotilainen <ryt...@gmail.com>

To unsubscribe from this group and stop receiving emails from it, send an email to nhibernate-develo...@googlegroups.com.

[Lauri Kotilainen]

Yeah... an actual case of "select is broken". Who would have thought.

Jira issue:

https://nhibernate.jira.com/browse/NH-3579

Pull request:

https://github.com/nhibernate/nhibernate-core/pull/244

I based the patch off the master branch -- figured that someone else will be a better judge of what to backport to earlier versions.

-Lauri

2013/12/15 Lauri Kotilainen <ryt...@gmail.com>

2013/12/12 Gunnar Liljas <gunnar...@gmail.com>

2013/12/11 Lauri Kotilainen <ryt...@gmail.com>

To unsubscribe from this group and stop receiving emails from it, send an email to nhibernate-development+unsubscri...@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.

[Роман Квасов]

Hello,

I too was hit by this bug.

Fix looks simple but it lay month without any activity.

Can I somehow help with this merge request?

2013/12/16 Lauri Kotilainen <ryt...@gmail.com>

To unsubscribe from this group and stop receiving emails from it, send an email to nhibernate-develo...@googlegroups.com.

[Lauri Kotilainen]

I can't speak for the core developer team, but I suppose you could try building the branch and smoke-testing it to see if it breaks anything. :)

-Lauri

[Oskar Berggren]

NhLinqExpression is instantiated in DefaultQueryProvider.PrepareQuery():

    protected NhLinqExpression PrepareQuery(Expression expression, out IQuery query, out NhLinqExpression nhQuery)
    {
        var nhLinqExpression = new NhLinqExpression(expression, Session.Factory);
        query = Session.CreateQuery(nhLinqExpression);
        ...

As I understand the code, it is the call to CreateQuery() that will lead to a query plan cache (QPC) lookup, and if no cached plan is found it will eventually call NhLinqExpression.Translate() before CreateQuery() returns. The proposed change, moves work from Translate() to the NhLinqExpression ctor. Unfortunately this effectively nullifies a large part of the purpose with the QPC, by having NH do the entire linq-to-hql-ast translation even before looking in the QPC.

If I'm right about the above, a simple change might be to just have the Translate() method nullify the _expression variable after it's done with it (and have it skip the translation code if ExpressionToHqlTranslationResults is already non-null).

Comments on this?

(On a somewhat tangential point, I feel the entire interaction between the linq implementation, QPC and the session implementation is rather messy code...)

/Oskar

2013/12/16 Lauri Kotilainen <ryt...@gmail.com>

To unsubscribe from this group and stop receiving emails from it, send an email to nhibernate-develo...@googlegroups.com.

[Oskar Berggren]

2013/12/11 Lauri Kotilainen <ryt...@gmail.com>

Here's the patch:

https://gist.github.com/rytmis/3735cfc274e135aae753

Unfortunately, even with that patch applied, the unit test in my other post fails -- even though a heap inspection with WinDBG confirms that the session is no longer rooted and is eligible for collection.

My attempts with dubious knowledge of Windbg seems to confirm that the test itself did indeed maintain a reference to the SessionImpl instance, as a local variable in the stack fram of IsGarbageCollected<TObject>(ref TObject @object, Action<TObject> useObject). I don't know how to explain that, given that the test code attempted to nullify the reference.

Anyway, I've merged a restructured test that ensures any stack frame that might reference the SessionImpl instance expires before the GC collection.

/Oskar

[Lauri Kotilainen]

On Saturday, January 18, 2014 11:44:53 PM UTC+2, Oskar Berggren wrote:

Unfortunately, even with that patch applied, the unit test in my other post fails -- even though a heap inspection with WinDBG confirms that the session is no longer rooted and is eligible for collection.

My attempts with dubious knowledge of Windbg seems to confirm that the test itself did indeed maintain a reference to the SessionImpl instance, as a local variable in the stack fram of IsGarbageCollected<TObject>(ref TObject @object, Action<TObject> useObject). I don't know how to explain that, given that the test code attempted to nullify the reference.

The second GC.Collect() pass fixed that for me, like Gunnar suggested. I'll take another look at it soon-ish to see if your modified test still behaves the way I think it should.

 

My first attempt at a fix actually was to discard the _expression reference. However, that didn't work -- IIRC because the results of the translation maintained another reference to the expression. Changing that would have had a considerably bigger change footprint, one that I wasn't comfortable with.

Another approach I tried was to replace the stored expression with something that wouldn't hold a session reference. Unfortunately, that only led to failing queries, largely due to the same reasons.

This change seems difficult to get right, which rather strongly supports your opinion about the messiness...

-Lauri

[Oskar Berggren]

2014/1/20 Lauri Kotilainen <ryt...@gmail.com>

On Saturday, January 18, 2014 11:44:53 PM UTC+2, Oskar Berggren wrote:

Unfortunately, even with that patch applied, the unit test in my other post fails -- even though a heap inspection with WinDBG confirms that the session is no longer rooted and is eligible for collection.

My attempts with dubious knowledge of Windbg seems to confirm that the test itself did indeed maintain a reference to the SessionImpl instance, as a local variable in the stack fram of IsGarbageCollected<TObject>(ref TObject @object, Action<TObject> useObject). I don't know how to explain that, given that the test code attempted to nullify the reference.

The second GC.Collect() pass fixed that for me, like Gunnar suggested. I'll take another look at it soon-ish to see if your modified test still behaves the way I think it should.

I added a bunch more, plus some other GC.xxx() methods, didn't help.

 

 

My first attempt at a fix actually was to discard the _expression reference. However, that didn't work -- IIRC because the results of the translation maintained another reference to the expression. Changing that would have had a considerably bigger change footprint, one that I wasn't comfortable with.

I later tried that too and discovered that Translate() is actually called twice and apparently produces different output on the second go. I haven't yet figured out what input data (or how) is modified to achieve this effect.

 

/Oskar

[Alexander I. Zaytsev]

Hi all,

I've made a pull request with a fix.

https://github.com/nhibernate/nhibernate-core/pull/248

Please review.

Best Regards, 

Alexander

2014/1/21 Oskar Berggren <oskar.b...@gmail.com>

[Роман Квасов]

Hi Alexander,

I think you forgot big chunk of leak, NhLinqExpression leak reference to constants that used in linq expression.

If linq query reference for object it end up in cache and object not released.

I wrote simple task case for demostration https://github.com/radiy/nhibernate-core/blob/master/src/NHibernate.Test/Linq/ExpressionSessionLeakTest.cs#L23.

I made over version of fix https://github.com/radiy/nhibernate-core/commit/82706daafcbfe4ec32aca94a51bbbb16369fe83e.

Please review.

2014/1/21 Alexander I. Zaytsev <haz...@gmail.com>

[cremor]

Hi Roman,

Is there still a leak in the latest master trunk even after the fix for NH-3579? If yes, please create a new Jira issue and GitHub pull request.

2014/1/21 Oskar Berggren <oskar.b...@gmail.com>

To unsubscribe from this group and stop receiving emails from it, send an email to nhibernate-development+unsub...@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.

--
 
---
You received this message because you are subscribed to the Google Groups "nhibernate-development" group.

To unsubscribe from this group and stop receiving emails from it, send an email to nhibernate-development+unsub...@googlegroups.com.

[Maarten]

His test case still works, can anyone confirm?

Op donderdag 27 maart 2014 08:44:40 UTC+1 schreef cremor: