Lambda business rule can cause a memory leak

Lambda business rule can cause a memory leak

Old forum URL: forums.lhotka.net/forums/t/11527.aspx


rfcdejong posted on Monday, August 13, 2012

With the help of ANTS i'm happy after finding this. 

It seems that the Lambda business rule has a risk if not used correctly. If you assign a local method of the business object as the rule parameter Action<RuleContext> then the BO won't be garbage collected as the _target of the Action is referencing the BO. If you do it in-code then it won't be a problem.

No memoryleak:

         protected override void AddBusinessRules()
         {
             base.AddBusinessRules();
             BusinessRules.AddRule(new Lambda((context) => { context.AddInformationResult("Test"); }));
         }

Memoryleak: 

         protected override void AddBusinessRules()         
         {
             base.AddBusinessRules();
             BusinessRules.AddRule(new Lambda((context) => { Test(context); }));
         }

        private void Test(Csla.Rules.RuleContext context)
        {
            context.AddInformationResult("Test");
        }

This will cause the _target inside the action to reference the BO.

 

Maybe this can be solved, to allow people to use a method as an action.

http://blog.catenalogic.com/post/2011/12/29/Using-true-weak-actions-without-causing-memory-leaks.aspx

 

JonnyBee replied on Monday, August 13, 2012

Wouldn´t it be better and easier to make the Test method static? 

Rules in CSLA is declared at the Type level and AddBusinessRules is only called once per Type of BusinessObject.
This means that the static method or instance method MUST be available for the duration of the application.

So I wouldn't call this a memory leak - the method MUST be available for ALL intances of this object type.
I'd rather call this a logical error by the developer.

If you create a weak link and the object instance gets disposed - this rule will ALWAYS fail as it cannot find the method.

We could maybe add a check to make sue the Action is to a static typed - but there is absolutely nothing wrong about having that rule method point to an instance method of a rule class.

rfcdejong replied on Tuesday, August 14, 2012

Yes, in this case it would be better to make the test method static and it is a logical error by the developer, but most developers do this kind of things by mistake. They know by now that they have to unbind events. In my case the lambda was  used to force a PropertyHasChanged and i was the one that had to dive into the strange behaviour...

When used wrong it will not only cause a memoryleak, the action holds a reference to the first instance of a business object, which means that a second instance of the same type of the business object will be missing the business rule. I think you know better and that AddBusinessRules is not called once per Type, but instead the BusinessRule anager is static. BusinessRules will be kept in memory for each business object type.

A check to make sure the Action is a to static method just blocks the road, perhaps there can be a check if it's a static method or instance method?

In case of a instance method, then the code below works. It's not holding a weak reference, but it creates a delegate and does a late-bound invoke.

In the constructor it creates a delegate.
var delegateType = typeof(OpenInstanceGenericAction<>).MakeGenericType(typeof(TParameter), targetType);
_action = Delegate.CreateDelegate(delegateType, null, action.Method);

 In the execute it does a late-bound invoke
_action.DynamiclyInvoke(target, parameter) 

 

JonnyBee replied on Tuesday, August 14, 2012

Well, in order to raise OnPropertyChanged you only need to add the property to AffectedProperties for that Property.

Actually - you second instance will call the lambda target method on the first object and in your case raise OnPropertyChanged on that property in the first object.!!

Yes. BusinessRuleManager maintains a static store of registered rules for ALL types.
IE: Per AppDomain, the AddBusinessRules method is called only ONCE for each ObjectType and then the rules is added to the static store.

And what the compiler does is actually:

         protected override void AddBusinessRules()
        {
            base.AddBusinessRules();
            BusinessRules.AddRule(new Lambda(Test));
        }

 

       private void Test(RuleContext context)
        {
            OnPropertyChanged(NameProperty);
        }

This code will only - ever - call OnPropertyChanged in the first BusinessObject.

JonnyBee replied on Tuesday, August 14, 2012

These 3 variants will make a dynamic call - when the developer is aware of it:

protected override void AddBusinessRules()
{
    base.AddBusinessRules();
    // using dynamic to invoke 
    BusinessRules.AddRule(new Lambda(IdProperty, (context) => ((dynamic) context.Target).Test(context)));
}
 
 
protected override void AddBusinessRules()
{
    base.AddBusinessRules();
    // using MethodCaller in Csla.Reflection
    BusinessRules.AddRule(new Lambda(IdProperty, 
(context) => MethodCaller.CallMethod(context.Target, "Test", context))); } protected override void AddBusinessRules() {     base.AddBusinessRules();     // using FasterFlect - http://fasterflect.codeplex.com/     BusinessRules.AddRule(new Lambda(IdProperty, (context) => context.Target.CallMethod("Test", context))); }
 
And adding code to check for static / non static is not so simple. 
Inline Lambdas (that work) is also non-static methods and one would have to check for:

rfcdejong replied on Tuesday, August 14, 2012

It is good to have a topic like this to make developers aware of it. Most developers just test the functionality just after starting the debugger so it won't raise until it has been deployed for test.

Lambda rule should be used with care, using one of the 3 variants.

Developers have to be aware, yes.

About the AffectedProperties, the business rule engine does not go n-level, while we have so many scenario's in our 'new' financial application that it must continue running business rules until no more values change.

On a side note:
In our abstraction layer code we implimented some sort of ChildRules which replace the OnChildChanged functionality, in there it's nearly the same as the BusinessRules with a ChildRulesManager with static ChildRules in the initialization AddChildRules, OnGetChildren _childChangedRules, OnDeserialize etc, except they get executed when a child property change or when a collection is changed. Almost all of the ChildChangedRules do something with the instance of the current BO as parent. In there the ChildChangedRules are constructed with a Action being wrapped by the WeakAction. This works.

 

Copyright (c) Marimer LLC