Possible memory leak in Geom SetBody()

Aug 31, 2009 at 11:24 AM

Hi all,

I just noticed a possible memory leak in the SetBody() method of the Geom class. Here is the more recent SVN code:

        public void SetBody(Body bodyToSet)
        {
            body = bodyToSet;

            bodyToSet.Updated += Update;
            bodyToSet.Disposed += BodyOnDisposed;

            Update(ref bodyToSet.position, ref bodyToSet.rotation);
        }

As you can see, the Geom hooks itself to the Updated events of the 'bodyToSet' without ever unhooking from those same events in the previous body. This can lead to memory leaks when changing the bodies of geoms dynamically (basically the delegates are left dangling).

Best regards,

Gonçalo

Coordinator
Aug 31, 2009 at 4:45 PM

They should be unhooked again in the dispose method if I remember correctly.

Sep 1, 2009 at 3:14 AM
Edited Sep 1, 2009 at 3:15 AM

Yes, they are unhooked in the Dispose method, but only on the body which is currently set. Here is the code of the current Dispose():

        protected virtual void Dispose(bool disposing)
{
if (!IsDisposed)
{
if (disposing)
{
//Make sure to remove the distancegrid when removing this geometry
if (PhysicsSimulator.NarrowPhaseCollider == NarrowPhaseCollider.DistanceGrid)
DistanceGrid.Instance.RemoveDistanceGrid(this);

//dispose managed resources
if (body != null)
{
//Release event subscriptions
body.Updated -= Update;
body.Disposed -= BodyOnDisposed;
}
}
}
IsDisposed = true;
}

If in the meantime you called SetBody() anywhere, Dispose() will only take care of unhooking events from the last body you setted.
All other bodies will be left with dangling handlers on their Update events.
Coordinator
Sep 1, 2009 at 12:28 PM

That is right. I'll take a look at it when I come home. Thanks for reporting it.

Sep 1, 2009 at 3:44 PM

Glopes, do you have a suggestion as to how to solve this? Do you think SetBody should check if the Geom's body is null, and if not, release the events automatically?

 

Sep 2, 2009 at 1:14 AM
Edited Sep 2, 2009 at 1:17 AM
Cowdozer wrote:

Glopes, do you have a suggestion as to how to solve this? Do you think SetBody should check if the Geom's body is null, and if not, release the events automatically?

 

Yes, I believe that's the intended semantics. The event handler is only meaningful as long as the Geom is related to a body, and since it's the Geom class which hooks the event in the first place, it should of course also be the one which unhooks it. Possibly you can refactor the code into a common method which gets called from both Dispose() and SetBody().

Or better yet, do it on SetBody() and on the Dispose() implementation just call SetBody(null). But maybe this is unwanted since the Geom class in farseer has no semantics in the case of a null body... better refactor it in a method I guess.

P.S.: While writing this suggestion I just noticed that SetBody() does not check whether the body argument is null. This can lead to unchecked NullReferenceException().

Sep 2, 2009 at 4:28 PM

Yes, but there are many places where a NullReferenceException could be thrown in the Farseer library. I think a large portion of those check are excluded to ensure good performance. Either way, checking doesn't give you much, since the only reasonable solution is to stop the game anyway. I've never seen Farseer throw a NullReferenceException because of itself, so we just need to make sure we're using Farseer properly and not giving it null references where it doesn't expect them.

Sep 2, 2009 at 5:39 PM

I'm not sure I agree with the performance argument on this one, since we're talking about an 'if' statement. I think there is much more that could be improved on Farseer in terms of algorithmic complexity and efficiency than hunting down error checks.

Of course I agree that one has to be careful while using a library, but on a really complex project there is always strange code paths leading to strange bugs at the strangest times. And sometimes without timely error checks they can be a nightmare to find.

Take this case, for example. Luckily SetBody() itself throws a null reference exception on null bodies while trying to hook to the events. But suppose it didn't for some reason. You could maybe detect the error in the middle of some obscure method called by the simulator's internals, and it could be the case that this would launch you on a wild goose chase in places which had nothing to do with the real error cause.

Unfortunately I've had my share of these obscure scenaria... :-(

Regardless, this is mainly just a ramble and I agree that Farseer is very stable and I sure have had no problems in dealing with the API so far :-)

Sep 2, 2009 at 8:04 PM

I agree with your opinion, but fortunately we're using C#, not C++, so uninitialized and null values will be caught either at compile time, or cause a straight-up crash rather than strange execution behaviour. It's true that the exception may be thrown from elsewhere, but if that was the case, I would probably start putting some of my own checks into the Farseer source to see where the first issue arrises.

There are definitely parts of Farseer where you need to read the source to see exactly what it is expecting for a parameter. Also, it's not always just an if statement that is needed. Doing null checks can turn a simple property getter/setter into a slightly more advanced one involving conditionals. There's a chance that the change will then cause the compiler to no longer inline the property getters and setters, so now you not only have a little extra logic check, but you have the overhead of a method call as well. Personally, I'd like some more Debug.Assert()'s scattered around, since they do a good job of documenting in code what is expected of parameters, and they "dissapear" when compiled in release mode. It's a broad topic though...