ResourcePtrs: disposal, thread-safety, implicit conversions

Hi there,

This will be a long post. Sorry for that. First an introduction:

ResourcePtrs in Ogre are reference-counted shared pointers, used for objects where implicit destruction is required. Mogre uses IDisposable to free managed ResourcePtrs, decreasing the reference-count on disposal. There is a ~MaterialPtr() destructor and a !MaterialPtr() finalizer on all the managed ResourcePtr classes.

If you forget to correctly dispose a ResourcePtr (which is a common mistake), the !MaterialPtr() finalizer is run on the finalizers thread, decreasing the reference-count of the TexturePtr. The problem is ResourcePtrs are not thread-safe unless OGRE_THREAD_SUPPORT is defined to be 1.

Moreover, when the reference-count of a resource reaches 0, the resource is destroyed. Decreasing the reference-count in a different thread may not be a big problem, destroying an object in a different thread may very well be. This is especially true of dynamically created resources (static resources are pretty much always referenced by the Ogre resource system, hence never destroyed).

So you should always keep your ResourcePtrs around while using them (instead of always fetching resources by name), and then dispose them you're done with them. Using statements are your friends.

There is a problem, though. Implicit conversions between ResourcePtrs and its descendants: MaterialPtr, TexturePtr, etc.
A common scenario is this (dynamically changing a texture in a material):
using (MaterialPtr mat = MaterialManager.Singleton.GetByName(materialName))
{
mat.GetBestTechnique().GetPass(0).GetTextureUnitState(0).SetTextureName("pic.jpg");
}
It seams OK. I've even gone to the trouble of disposing the MaterialPtr. The problem is GetByName returns a ResourcePtr. That ResourcePtr is implicitly converted to a MaterialPtr and forever lost. It will eventually be garbage collected, and finalized in the finalizers thread.

This will lead to crashes, particularly when mixed with dynamic resources. The only way to deal with this is the following:
using (ResourcePtr res = MaterialManager.Singleton.GetByName(template.Material))
using (MaterialPtr mat = res)
{
mat.GetBestTechnique().GetPass(0).GetTextureUnitState(0).SetTextureName("pic.jpg");
}
To avoid this (which is even more error prone), I've decided to change the way Mogre works. Implicit conversions of ResourcePtrs transfer ownership of said ResourcePtrs. When converting from a ResourcePtrs to a MaterialPtr, I invalidate the original ResourcePtr.

Here is Mogre:static MaterialPtr^ FromResourcePtr( ResourcePtr^ ptr )
{
return (MaterialPtr^) ptr;
}

static operator MaterialPtr^ ( ResourcePtr^ ptr )
{
if (CLR_NULL == ptr) return nullptr;
void* castptr = dynamic_cast<Ogre::Material*>(ptr->_native);
if (castptr == 0) throw gcnew InvalidCastException("The underlying type of the ResourcePtr object is not of type Material.");
return gcnew MaterialPtr( (Ogre::MaterialPtr) *(ptr->_sharedPtr) );
}

Here is my patched Mogre:static MeshPtr^ FromResourcePtr( ResourcePtr^ ptr )
{
if (CLR_NULL == ptr) return nullptr;
void* castptr = dynamic_cast<Ogre::Material*>(ptr->_native);
if (castptr == 0) throw gcnew InvalidCastException("The underlying type of the ResourcePtr object is not of type Material.");
return gcnew MaterialPtr( (Ogre::MaterialPtr) *(ptr->_sharedPtr) );
}

static operator MeshPtr^ ( ResourcePtr^ ptr )
{
MeshPtr^ res = FromResourcePtr(ptr); // convert as we did before
delete ptr; // invalidate previous pointer
return res; // return converted pointer
}

In the generator this would be implemented by changing Wrapper.cs around line 960 accordingly.
Does anyone object to the change, would you guys like a patch?

Thank you all.

yeah~~，great. i agree your point.

we have some strange crash,i think shared ptr cause crash.
Mogre has some problem in sharedptr manage.

export ogre as C style function ,then use p/inovke will be more stable i think.

Great post ncruces, thank you! Coincidentally, I went through a pretty big application a few days before your post, to replace all the XYZPtr with the ResourcePtr/XYZPtr combo, so yes, it would be great if Mogre would take care of this by itself.

Well, I've never found a situation where I want to keep track of the ResourcePtr after converting it to a MaterialPtr (beyond keeping it around just so I can later correctly dispose it). So, as long as we're implicitly converting the ResourcePtr to a MaterialPtr, I think we can also implicitly destroy the original ResourcePtr as well.

Does everyone agree? If so, I've attached a patch. Anyone with commit rights is welcome to apply it in Mercurial, and give it a go.


A few further notes.

If you were already doing the right thing (i.e. keeping the ResourcePtr around to dispose it), this doesn't cause a problem: double disposal of the ResourcePtr (implicit conversion disposes it, then you do too) is perfectly safe.

If (for some reason) you need the previous semantics, FromResourcePtr is still around to do the job. I only changed implicit conversions.

The only place I can foresee this behaving strangely, is if you pass your ResourcePtr as an argument to a function taking (say) a MaterialPtr. The ResourcePtr is implicitly converted and invalidated, and you can't keep using it. As I've said, I've never encountered this pattern myself. And you can always use MaterialPtr.FromResourcePtr(ptr) instead.

Thank you for posting. I guess it would take weeks to find such an error
And if I am right, Beauty has write permissions for the repository.

Incredible work ncruces. How did you discover these issues? Did you run some sort of memory profiler over your application or you just have experience similar issues in the past?

Incredible work ncruces. How did you discover these issues?
I got some random crashes. After coping with them for a couple of years, we traced some of those to our use of dynamically generated materials.

The most common scenario for me is using anim textures for certain effects. If I apply the same material to multiple objects they'll all be in sync, so I'll usually clone those materials on usage (not the best solution, but does the job).

If we leave it to the finalizer to dispose the MaterialPtr it will be run on the finalizer thread. If the finalizer gets to run while we're still using the material, it's not a big deal (all it's doing is "refcount--"). If however you're no longer using the material by then, it will attempt to destroy the material in the finalizer thread. This is is a big no-no, and will eventually lead to a crash.

Also, when you close a Mogre application, .NET will exit every thread, run every "finally" block, and dispose objects in every "using" block. Then, it will run finalizers on all remaining finalizable objects in the finalizer thread. By then, Mogre's root object will be disposed, and you'll be trying to dispose random ResourcePtr's in a different thread, after Mogre has been shutdown. This will lead to random crashes on application shutdown.

Diposing your objects judiciously (which is easier said than done) will prevent these, and other weird bugs.

If we leave it to the finalizer to dispose the MaterialPtr it will be run on the finalizer thread. If the finalizer gets to run while we're still using the material, it's not a big deal (all it's doing is "refcount--"). If however you're no longer using the material by then, it will attempt to destroy the material in the finalizer thread. This is is a big no-no, and will eventually lead to a crash.
Forgive me if I'm wrong, but is the IDisposable pattern implemented properly on the MaterailPtr? If I understand it correctly the finalizer should call Dispose safely in the finalizer even if the object has already been disposed.

http://www.codeproject.com/Articles/153 ... -Pattern-P
http://stackoverflow.com/questions/5380 ... -interface

Also, when you close a Mogre application, .NET will exit every thread, run every "finally" block, and dispose objects in every "using" block. Then, it will run finalizers on all remaining finalizable objects in the finalizer thread. By then, Mogre's root object will be disposed, and you'll be trying to dispose random ResourcePtr's in a different thread, after Mogre has been shutdown. This will lead to random crashes on application shutdown.
Would it be possible to detect invalid ResourcePtr's? For example, when they are disposed they could be flagged (maybe set them to null or zero) so that the root object won't try to dispose them a second time.

Again, forgive me if I'm completely out of the ball park here. I totally agree we should all be good little programmers and dispose our objects correctly, but I hate the idea that they won't be disposed if I forget. If I had time I would like to take a look into the Mogre source code and see what's going on. You've probably already done that

This sounds like an OK solution.

We too had some really strange crashes and material finalizer issues until I realized the casting between ResourcePtr and MaterialPtr wasn't working quite right, after taking a quick look at the MOgre sources.

I replaced all those places, as you did, with the:


using (var pRes = GetByName(whatever))
using (var thingy = ThingyPtr.FromResourcePtr(pRes))
{
// Do stuff
}


BTW, I'm wondering if that code snippet should be sticky/wiki article somewhere, because straight casting is, basically, totally wrong right now. Maybe something like "looking up resources by name or handle correctly" or something...

Anyway, that change fixed a lot of problems.

If you are going to patch MaterialPtr, you should probably patch all the others as well (I had to do something similar for our TexturePtr references as well).

The only problem I see is if someone had existing code like:


using (var pRes = GetByName(whatever))
{
var thingy = (ThingyPtr)pRes;

// Some work

var thingy2 = (ThingyPtr)pRes;

// Uh-oh!!! thingy2 is now null... why did that happen?!?
}


But I guess if the pointer goes null, those errors might be easy to track down. I guess it might be a little weird if you didn't realize what was going on that a cast made the castee go null...

Forgive me if I'm wrong, but is the IDisposable pattern implemented properly on the MaterailPtr? If I understand it correctly the finalizer should call Dispose safely in the finalizer even if the object has already been disposed.
Yes, it is. It is implemented as properly as it can ever be. The problem is finalizers run in a different thread, and (M)OGRE isn't thread safe.

Disposing the MaterialPtr works just fine, even in a different thread (most of the time). The problem is disposing the MaterialPtr may cause the Material it points (a MaterialPtr is a pointer to a Material) to be destroyed. Doing that in a different thread will cause crashes.

Would it be possible to detect invalid ResourcePtr's? For example, when they are disposed they could be flagged (maybe set them to null or zero) so that the root object won't try to dispose them a second time.
Again, Mogre's Dispose methods are implemented as properly as they can ever be.

The problem is code like this one I have somewhere:
protected override void Dispose(bool disposing)
{
if (!disposed)
{
if (disposing)
{
var root = Root.Singleton;
if (root != null)
{
if (texture != null) // this is a TexturePtr
{
TextureManager.Singleton.Remove(name);
texture.Dispose();
texture = null;
}
}
disposed = true;
}
}

base.Dispose(disposing);
}

This will crash on shutdown, sometimes. Why? Because texture (the managed pointer wrapper) might not be null, yet the unmanaged object it points to might already be null. So you have to write this (which is "unsafe"):
if (texture != null && texture.NativePtr != (void*)0) ...

The fix for this, is an operator== overload on Wrapper (and an updated operator== overload on the bunch of classes that already have one). I'm currently using this:
static bool operator == (Wrapper^ val1, Wrapper^ val2)
{
if ((Object^)val1 == (Object^)val2) return true;
if ((Object^)val1 == nullptr) return val2->_native == 0;
if ((Object^)val2 == nullptr) return val1->_native == 0;
return (val1->_native == val2->_native);
}

static bool operator!=(Wrapper^ a, Wrapper^ b)
{
return !(a == b);
}

This sounds like an OK solution.

We too had some really strange crashes and material finalizer issues until I realized the casting between ResourcePtr and MaterialPtr wasn't working quite right, after taking a quick look at the MOgre sources.

I replaced all those places, as you did, with the:

using (var pRes = GetByName(whatever))
using (var thingy = ThingyPtr.FromResourcePtr(pRes))
{
// Do stuff
}

BTW, I'm wondering if that code snippet should be sticky/wiki article somewhere, because straight casting is, basically, totally wrong right now. Maybe something like "looking up resources by name or handle correctly" or something...

Anyway, that change fixed a lot of problems.
Right, that was what I doing. And I agree, it should be on a sticky/wiki.
It's still pretty easy to forget to always do that, hence my attempt to come up with a "better" alternative.

If you are going to patch MaterialPtr, you should probably patch all the others as well (I had to do something similar for our TexturePtr references as well).
Sure, I patched them all, internally. I've also patched Mogre's code generator, a patch I meant to have attached to my previous forum post. I see I went to all the trouble to isolate that patch, just to forget to attach it.
Let me find it, and I'll post it here. I'll also post my operator== thing, which I think is useful as well, and hasn't broken anything for me.

The only problem I see is if someone had existing code like:

using (var pRes = GetByName(whatever))
{
var thingy = (ThingyPtr)pRes;

// Some work

var thingy2 = (ThingyPtr)pRes;

// Uh-oh!!! thingy2 is now null... why did that happen?!?
}

But I guess if the pointer goes null, those errors might be easy to track down. I guess it might be a little weird if you didn't realize what was going on that a cast made the castee go null...
Sure, those are the pitfalls. All I can say is, I've been using this for about a year, with a team of 3. In that year, this reduced the number of times we forgot to properly dispose stuff, and we have never found the pitfalls in actual code. 99.9% of the time, we cast the ResourcePtr to the actual ThingyPtr, and use that instead, so this makes it easier. That's my experience.

This will crash on shutdown, sometimes. Why? Because texture (the managed pointer wrapper) might not be null, yet the unmanaged object it points to might already be null. So you have to write this (which is "unsafe"):
if (texture != null && texture.NativePtr != (void*)0) ...
We could add an "bool IsPointerNull (...)" function, which checks the pointer.

Or add a static function to ResourcePtr(my favorite):
static bool IsNullOrDiposed(ResourcePtr resPtr) { return (resPtr= null || resPtr._sharedPtr = 0); }
This is similar to String::IsNullorEmpty(...). Easy and simple.

Can't attach patches it seems (doesn't accept any extension I've tried (.txt, .patch, .diff). I'll past it here:

diff -r c2be4e87dc8b Codegen/AutoWrap/Meta/Wrapper.cs
--- a/Codegen/AutoWrap/Meta/Wrapper.cs Wed Apr 18 02:28:03 2012 +0200
+++ b/Codegen/AutoWrap/Meta/Wrapper.cs Sat Aug 18 18:18:22 2012 +0100
@@ -959,17 +959,21 @@

sb.AppendLine("static " + type.Name + "^ FromResourcePtr( ResourcePtr^ ptr )");
sb.AppendLine("{");
- sb.AppendLine("\treturn (" + type.Name + "^) ptr;");
+ sb.IncreaseIndent();
+ sb.AppendLine("if (CLR_NULL == ptr) return nullptr;");
+ sb.AppendLine("void* castptr = dynamic_cast<" + nativeClass + "*>(ptr->_native);");
+ sb.AppendLine("if (castptr == 0) throw gcnew InvalidCastException(\"The underlying type of the ResourcePtr object is not of type " + baseClass + ".\");");
+ sb.AppendLine("return gcnew " + type.Name + "( (" + type.FullNativeName + ") *(ptr->_sharedPtr) );");
+ sb.DecreaseIndent();
sb.AppendLine("}");
sb.AppendLine();

sb.AppendLine("static operator " + type.Name + "^ ( ResourcePtr^ ptr )");
sb.AppendLine("{");
sb.IncreaseIndent();
- sb.AppendLine("if (CLR_NULL == ptr) return nullptr;");
- sb.AppendLine("void* castptr = dynamic_cast<" + nativeClass + "*>(ptr->_native);");
- sb.AppendLine("if (castptr == 0) throw gcnew InvalidCastException(\"The underlying type of the ResourcePtr object is not of type " + baseClass + ".\");");
- sb.AppendLine("return gcnew " + type.Name + "( (" + type.FullNativeName + ") *(ptr->_sharedPtr) );");
+ sb.AppendLine(type.Name + "^) res = FromResourcePtr(ptr);");
+ sb.AppendLine("delete ptr;");
+ sb.AppendLine("return res;");
sb.DecreaseIndent();
sb.AppendLine("}");
sb.AppendLine();



Or add a static function to ResourcePtr(my favorite):
static bool IsNullOrDiposed(ResourcePtr resPtr) { return (resPtr= null || resPtr._sharedPtr = 0); }
This is similar to String::IsNullorEmpty(...). Easy and simple.

Yes, that would do as well. Any thing would, as far as I'm concerned, as long as it doesn't require unsafe code everywhere.
But this should be added to Wrapper actually, as it affects all wrapped objects, not ResourcePtrs.
I used to add an extension method to Wrapper somewhere in my own code, but this should be done in Mogre proper.

Something like this:
static bool IsNullOrDisposed(Wrapper^ obj)
{
return (obj == nullptr || obj->_native == 0);
}

ncrues, your patch has a wrong ")":
sb.AppendLine(type.Name + "^) res = FromResourcePtr(ptr);");
should be
sb.AppendLine(type.Name + "^ res = FromResourcePtr(ptr);");
I've built an Mogre 1.7.4 with your (corrected) patch and:

1. Wrapper.IsEmptyOrDisposed
   Wrapper.SafeDispose[/list:u]
   
   SafeDispose (an experiment):
   static bool SafeDispose(Wrapper^ obj)
{
if (obj) //Maybe IsEmptyOrDisposed instead?
{
obj->~Wrapper();
return true;
}
else return false;
}
   Possible usage:
   If (Wrapper.SafeDispose(MyMaterial)) MyMaterial = null;
   Disposes the resource and returns TRUE on success.
   Equal to:
   if (obj != null)
{
obj.Dispose();
obj=null;
}
   By the way:
   Could it be, that FileInfoList(Ptr) and StringVectorList(Ptr) have the same (or related) issue? I had some errors with these some time ago. I "fixed" this by suppressing the finalizer.

As long as we are talking about general strategies, I do something a bit different to handle the Dispose cases.

1. *Ptr are never allowed as field members of a class. We store the string name, and if we need a *Ptr we look it up (and dispose it) inside the function.
2. For the finalization path, we always post back to the main thread, we have utility code like this:


static readonly System.Threading.SynchronizationContext SyncContext;
internal static void PostAction<T>(Action<T> action, T data)
{
if (!Core3D.IsDisposed) // Only if we aren't shutting down
SyncContext.Post(o => action((T)o), data);
}
static Core3D()
{
SyncContext = System.Threading.SynchronizationContext.Current;
if (SyncContext == null)
SyncContext = new System.Threading.SynchronizationContext();
}


and then code like this in our own material class:


protected virtual void Dispose(bool bDisposing)
{
if (_materialName != null)
{
if (bDisposing)
{
MaterialManager.Singleton.Remove(_materialName);
}
else
{
// We do things to MOgre which might eventually do things to DirectX, and DirectX does not like doing things
// to stuff in a different thread than the one it was created in, so we'll post here and process the removal later
Core3D.PostAction(matname => MaterialManager.Singleton.Remove(matname), _materialName);
}

_materialName = null;
}
}


Oh, oh, very interesting! After some (short) studies on codeproject for SynchronizationContexts, I have one question:
If I am right, you "safe" the main thread's sync context and queue work for it using "post(delegate, state)" from other threads.
If there is no sync context, you create a new one.

static Core3D()
{
SyncContext = System.Threading.SynchronizationContext.Current;
if (SyncContext == null)
SyncContext = new System.Threading.SynchronizationContext();
}


But why do you do not attach it to the main thread?
And the default implementation of SynchronizationContext.Post is:
public virtual void Post(SendOrPostCallback d, object state)
{
ThreadPool.QueueUserWorkItem(new WaitCallback(d.Invoke), state);
}
So the delegate is executed in some thread but not the main thread?!

The static Core3d() ctor is run on the main thread (the first reference to Core3d happens then), and that that point we "capture" the SynchronizationContext from the main thread and store it in our static Core3d.SyncContext field.

Now our app is a Windows Forms app, which means that on the main thread, System.Threading.SynchronizationContext.Current will be a System.Windows.Forms.WindowsFormsSynchronizationContext, which is a specialization that does Post and Send calls to dispatch on the windows message pump (which pumps in the main thread), meaning that will always execute back on the main thread.

The null check in the Core3d() ctor is mainly there just in case System.Threading.SynchronizationContext.Current doesn't return anything, though I believe in our app's standard code flow that actually never happens, it always has a WindowsFormsSynchronizationContext sitting in it.

Thanks.
Just one more question, why do you use Post(...) and not Send(...)? Both execute the delegate in the attached thread.
Post is asynchron and would'nt cause the delayed deletion problems, if you want to remove and recreate a resource?

The null check in the Core3d() ctor is mainly there just in case System.Threading.SynchronizationContext.Current doesn't return anything, though I believe in our app's standard code flow that actually never happens, it always has a WindowsFormsSynchronizationContext sitting in it.
Its useless because the default implementation of SynchronizationContext executes the delegate in the caller's thread (and no the main thread) (see the .Net source or my last post).

Yeah, Send() might be better there than Post()... though for our particular code path, that Post() is only happening when the object is getting finalized, so we only really need lazy removal, we won't be re-creating something of the same name anytime soon. Certainly, if you needed something of the same name right away Send() is probably the better option there... although if you do Send() on the finalizer thread, that will block the finalizer, and (at least for WinForms), if the app is shutting down, the message pump may not be pumping, so the Send() will never complete, and you might have problems.

For the synchronization context, you are correct. I believe Send() just calls it immediately and Post() executes on some arbitrary thread.