Memory Leak in Expression Clone

[pradeepbkkumar]

To increase the performance of expression evaluation, we follow the prototype design pattern. We pre-parse all the expressions when our application starts, and keep them as prototypes in memory. When we get user requests, We clone the appropriate expression instance (prototype), change it's variables and evaluate. It saves us lot of time (20x faster).

However, When we clone the expression, the cloned expression context is not used when cloning it's member variables - expression options, parser options, variable collection, etc., So all these cloned variables (new instances) still refers to the old expression context (expression context of prototype) and there's an event handler which makes this old expression context refer it back to these cloned variables, causing a huge memory leak.

Referring to the below code, Options, ParserOptions, Imports and even the VariableCollection has reference to the old expression context.

        internal ExpressionContext CloneInternal(bool cloneVariables)
        {
            ExpressionContext context = (ExpressionContext)this.MemberwiseClone();
            context._myProperties = _myProperties.Clone();
            context._myProperties.SetValue("Options", this.Options.Clone());
            context._myProperties.SetValue("ParserOptions", this.ParserOptions.Clone());
            context._myProperties.SetValue("Imports", this.Imports.Clone());
            context.Imports.SetContext(context);

            if (cloneVariables == true)
            {
                context._myVariables = new VariableCollection(this);
                this.Variables.Copy(context._myVariables);
            }

            return context;
        }

And the below constructor (of VariableCollection) has an event handler HookOptions which makes the prototype expression context to refer to this cloned variable collection. So, the prototype expression context end up referring to all cloned variable collections causing the memory leak.

        internal VariableCollection(ExpressionContext context)
        {
            _myContext = context;
            this.CreateDictionary();
            this.HookOptions();
        }

        #region "Methods - Non Public"

        private void HookOptions()
        {
            _myContext.Options.CaseSensitiveChanged += OnOptionsCaseSensitiveChanged;
        }

Kindly look into this and fix it.

Fix would be just setting the right (cloned) expression context when cloning it's member variables. Something like below

         internal ExpressionContext CloneInternal(bool cloneVariables)
        {
            ExpressionContext context = (ExpressionContext)this.MemberwiseClone();
            context._myProperties = _myProperties.Clone();
            context._myProperties.SetValue("Options", this.Options.Clone(context));
            context._myProperties.SetValue("ParserOptions", this.ParserOptions.Clone(context));
            context._myProperties.SetValue("Imports", this.Imports.Clone(context));
            context.Imports.SetContext(context);

            if (cloneVariables == true)
            {
                context._myVariables = new VariableCollection(context);
            }

            return context;
        }

[pradeepbkkumar]

I tried creating a pull request with the fix, but I don't have write access to this repo.

[Liwoj]

Haha, funny thing is I reported the very same bug in the original repo almost 11 years ago

I ended up using pretty simple implementation of pooling cache (cloning expressions only when there is last one in the pool and returning them back after use ...so it is kind of memory leak still but at least the number of cloned expressions is under control and they are ready to be used again instead of just taking memory)

BTW I think your fix is missing the variables copy this.Variables.Copy(context._myVariables);