Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak named parameter holds entity references #3030

Closed
philiphoy opened this issue Mar 7, 2022 · 6 comments · Fixed by #3144
Closed

Memory leak named parameter holds entity references #3030

philiphoy opened this issue Mar 7, 2022 · 6 comments · Fixed by #3144

Comments

@philiphoy
Copy link

Investigating a memory leak in an app, it was evident that the NHibernate.Engine.Query.QueryPlanCache was holding on to references to entities via instances of NHibernate.Param.NamedParameter. This only happens if the linq query expression embeds the entity itself as a parameter.

Below is a complete example which demonstrates this behaviour, note that only the first time a unique query is used is the entity pinned, see var first_reference_is_not_null = firstReference.Target; :

using FluentNHibernate.Cfg;
using FluentNHibernate.Cfg.Db;
using FluentNHibernate.Mapping;
using NHibernate;
using NHibernate.Dialect;
using NHibernate.Tool.hbm2ddl;

public class programm
{
    public static void Main()
    {
        WeakReference sessionReference = null;
        WeakReference firstReference = null;
        WeakReference secondReference = null;

        new Action(() =>
        {
            var session = ConfigureSessionFactory();

            var first = new Test() { Id = 1 };
            var second = new Test() { Id = 2 };

            var f = session.Query<Test>().FirstOrDefault(f => f == first);
            var s = session.Query<Test>().FirstOrDefault(f => f == second);


            sessionReference = new WeakReference(session, true);
            firstReference = new WeakReference(first, true);
            secondReference = new WeakReference(second, true);

            session.Dispose();
        })();

        GC.Collect();
        GC.WaitForPendingFinalizers();

        var session_reference_is_null = sessionReference.Target;
        var first_reference_is_not_null = firstReference.Target;
        var second_reference_is_null = secondReference.Target;
    }

    private static ISession ConfigureSessionFactory()
    {
        var configuration = Fluently.Configure().Database(SQLiteConfiguration.Standard.InMemory).Mappings(m =>
        {
            m.FluentMappings.AddFromAssemblyOf<Test>();
        });

        configuration.ExposeConfiguration(c => SchemaMetadataUpdater.QuoteTableAndColumns(c, new MsSql2012Dialect()));

        var config = configuration.BuildConfiguration();

        var factory = config.BuildSessionFactory();
        var session = factory.OpenSession();
        var wr = new StringWriter();

        new SchemaExport(config).Execute(true, true, false, session.Connection, wr);
        var t = wr.ToString();
        return session;
    }

    public class TestMap : ClassMap<Test>
    {
        public TestMap()
        {
            Id(x => x.Id).GeneratedBy.Assigned();
        }
    }

    public class Test
    {
        public virtual int Id { get; set; }
    }
}
@hazzik
Copy link
Member

hazzik commented Apr 7, 2022

Sounds correct. What should we do?

@philiphoy
Copy link
Author

Release the reference to the entity once the session is closed perhaps.

@bahusoid
Copy link
Member

What should we do?

We can store proxy in cache.

@fredericDelaporte
Copy link
Member

We can store proxy in cache.

Meaning, replace the entity instances we see in parameters with new uninitialized proxy instances? Those proxies would have to be detached from the session, otherwise uniqueness of entities residing inside the session would be violated. What should we do if proxies are not enabled? With composite key entities with their id not mapped as a component? (There are issues for proxifying them.)

I wonder why we need them in the query cache in the first place. Would it not be possible to nullify the parameter value in the cache?

hazzik added a commit to hazzik/nhibernate-core that referenced this issue Aug 30, 2022
hazzik added a commit to hazzik/nhibernate-core that referenced this issue Aug 30, 2022
hazzik added a commit to hazzik/nhibernate-core that referenced this issue Aug 30, 2022
@hazzik hazzik added this to the 5.3.13 milestone Aug 30, 2022
hazzik added a commit to hazzik/nhibernate-core that referenced this issue Aug 30, 2022
@hazzik
Copy link
Member

hazzik commented Aug 30, 2022

I wonder why we need them in the query cache in the first place.

Turns out we do not need them.

Named parameters are generated during initial phase of linq translation (in the constructor of NhLinqExpression). Then they are used during translation, but Value of the parameter is not used. Only Type and IsGuessedType properties are used. After that we do not need the parameters at all, because provider will be using the parameters of each query instead of the cached version.

I've decided to fix it in 5.3.13 because part of the problem was introduced in #2365 (5.3.0)

hazzik added a commit that referenced this issue Aug 31, 2022
Cache only required parts of NhLinqExpression in QueryExpressionPlan

Fixes #3030
@hazzik
Copy link
Member

hazzik commented Aug 31, 2022

Fixed in #3144 (5.3.13)

@hazzik hazzik closed this as completed Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants