Memory leak bug found in BusinessBase.cs

Memory leak bug found in BusinessBase.cs

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


KGy posted on Thursday, November 20, 2008

I use the most recent version of CSLA for .NET 2.0 (that is v2.1.4) and I have discovered the following thing:

Lets assume that an editable child list is bound to a grid/listbox/etc. To edit a child item I open a new form and to make possible to cancel changes that were performed on child I use undo possibilities like this:

public bool Execute(Child child)
{
    // binding (unbind is performed on closing the dialog)
    this.Child = child;

    // editing
    child.BeginEdit();
    if (this.ShowDialog() == DialogResult.OK)
    {
        child.ApplyEdit();
        return true;
    }
    else
    {
        child.CancelEdit();
        return false;
    }
}


So when the form is opened the child element is bound twice: firstly, in the grid of the opener form, and secondly, in the newly opened form. Seemingly everything works fine: thanking to data binding as I modify properties of the child I can see modifications in the grid as well, and when I cancel editing, modifications will be undone. But in practice there is a memory leak when CancelEdit is called. I have noticed that EditLevel of child is increases on calling BeginEdit, decreases on AppyEdit but does not decrease on calling CancelEdit! It means that the stack that stores undo steps gets greater and greater.

Source of the problem: When CancelEdit is called (which at first correctly undoes the object and decreases EditLevel) a _bindingEdit flag will be cleared in UndoChangesComplete method. This method calls OnUnknownPropertyChanged at the end, which invokes some delegates which finally call IEditableObject.BeginEdit via interface. Since _bindingEdit was cleared (despite of the fact that the child is bound in the caller form, too) this will call the regular BeginEdit immediately after the object has been undone. This bug is hard to notice because functionally child objects work well and if EditLevel of the root object is correct, then it will be savable along with its children.

Here is my solution for the problem (old version in comments, new version is in italic; only BusinessBase.cs is affected):

Line 629:
private /*bool*/ int _bindingEdit = 0;

Line 640: (this is when only the first call should be honored and this was the source of the problem)
void System.ComponentModel.IEditableObject.BeginEdit()
{
  if (/*!*/_bindingEdit == 0)
    BeginEdit();
}


Line 658: (in IEditableObject.CancelEdit)
if (_bindingEdit > 0)

Line 685: (in IEditableObject.EndEdit)
if (_bindingEdit > 0)

Line 713: (in BeginEdit)
_bindingEdit++; // = true;

Line 741: (in UndoChangesComplete - this triggered the problem by clearing the flag)
_bindingEdit--; // = false;

Line 766: (in ApplyEdit)
_bindingEdit--; // = false;

This seems to be a good solution for me but of course I cannot be sure whether this causes another problem somewhere else. Smile [:)]

Cheers,
  KGy

ajj3085 replied on Thursday, November 20, 2008

Actually, the most recent version for .net 2.0 is 3.0.5.  I strongly recommend you upgrade to that, because there are a TON of data binding fixes.

KGy replied on Thursday, November 20, 2008

Oh, I don't know why but I thought that CSLA 3.x is for .NET 3.x...
My fault.

ajj3085 replied on Thursday, November 20, 2008

It's pretty reasonable to expect that actually.. but as .net 3.0 was only an additive update, so is Csla 3.0.x.  Csla 3.5 requires .net 3.5 though, because the compilers for .Net have changed.

Oh, I think you'll need to set a NET20 compiler flag when using Csla 3.0.x though, to disable the compiling of features that require .net 3.0.

KGy replied on Thursday, November 20, 2008

I have just installed CSLA 3.0.5 and I have to say that this version contains the bug, too. Functionally everything is OK but it is possible to reproduce the memory leak with the scenario that I have shown in my first post.

Shortly:
- ApplyEdit is fine, EditLevel decreases
- CancelEdit has still the wrong behaviour: UndoChages works fine until the UndoChangesComplete call, where _bindingEdit is cleared, so the final OnUnknownPropertyChanged call results in a new BeginEdit invocation if the object is bound to more than one sources. I emphasize that functionally there is no problem because the successfully undone object will be copied to the stack again. And since calling Save on the root object checks only the EditLevel of the root object the bug cannot be noticed unless you get an OutOfMemoryException. Smile [:)]

Problem is not too serious because as soon as you realod the whole business object hierarchy (for example after saving) and release the old references the "leak" disappears.

The solution is almost the same as I have sketched above but in this version also UndoableBase.cs is affected since _bindingEdit field has been moved into that class (and a new BindingEdit property has been introduced).

ajj3085 replied on Thursday, November 20, 2008

Ok, I re-read your post in more detail now..

I think the issue is that what you want to do isn't really supported; Windows BindingSource components expect that a single bindingsource is "in charge" of the object it is managing.  In your case, you have two; one bindingsource managing the child via the grid, and another on the form which you open.  That's not supported by Windows data binding...

You might be able to work around this issue though; you should be able to chose to ignore Windows databinding calls, and manage the undo / cancel / accepts yourself.  Look for DisableIEditableObject.  There's a thread on this forum as well, you should try searching for that.

HTH
Andy

KGy replied on Thursday, November 20, 2008

> That's not supported by Windows data binding...

Windows handles multiple bounded sources well - just try to bind the same collection to a grid, list, combo box, etc. Windows handles both currency (select a row in the grid and also everything else will update the selection) and data (the edited field will be refreshed everywhere). As far as I know what WinForms does not handle correctly is reading back the previously set data into the edited control itself. But CSLA BindingSourceRefresh control solves this issue.

RockfordLhotka replied on Thursday, November 20, 2008

Data binding allows binding the same collection to multiple UI controls, but not multiple bindingsource controls. At least not if the objects in the collection implement IEditableObject (which CSLA objects do).

In other words, you can bind multiple UI elements (grid, list, etc) to a bindingsource, and that bindingsource to an object. But if you bind multiple bindingsource controls to the same underlying object (and that object implements IEditableObject) then things break quickly.

The reason is that the bindingsource control assumes it "owns" that object. It calls the IEditableObject methods as it needs, assuming that no other entity (like another bindingsource control) will be calling them as well.

For example, as soon as a bindingsource binds to an object, it calls IEO.BeginEdit(). If you connect a second bindingsource to that same object, it will also call IEO.BeginEdit() - but that second call is ignored due to the rules of implementing IEO (only the first call is to be honored).

If the user then starts doing things through one bindingsource, it'll call EndEdit() or CancelEdit() as appropriate, followed by a subsequent BeginEdit() call. The other bindingsource may also react, based on PropertyChanged or ListChanged events from the object, and the two controls can easily end up fighting each other.

RockfordLhotka replied on Thursday, November 20, 2008

I see, I more carefully read your initial post. That's interesting, in that you are proposing a solution to the race condition I outlined in my previous post.

Your solution seems like a good one, at least on the surface. Whether it introduces unwanted side-effects is the real question :)

I'll add this to the issue tracker - something to investigate for 3.6.1 at least.

KGy replied on Friday, November 21, 2008

Unfortunately the solution for version 3.0.5 is not as trivial as for version 2.x that I have written in my first post. Simply converting the bool flag into an integer field and increasing/decreasing it instead of setting/clearing will not do the trick because of the restructured logic has been made since 2.x. I have sent you a solution in email that seems good to me - maybe you can use it as a start up point in the issue tracker.

Copyright (c) Marimer LLC