sharpdx / SharpDX

SharpDX GitHub Repository
http://sharpdx.org
MIT License
1.69k stars 641 forks source link

DataStream doesn't follow Dispose() pattern regarding the unmanaged references #996

Closed aienabled closed 6 years ago

aienabled commented 6 years ago

Hello!

DataStream has a _gCHandle field which will be never released for abandoned (collected with GC) instance as there is no finalizer defined.

I've noticed a memory leak in MonoGame related to this issue in the way I use SoundEffect class from here (by keeping it abandoned and relying on GC). I understand that DataStream might have been never intended to be used the way it's used there and that if used with using or try-finally patterns controlling auto disposal it should not be a problem, however it's a public API of SharpDX and so there should be never a possibility for memory leaks related to the unmanaged resources. Also, I see no point of having a properly implemented Dispose(disposing) method when it's never called with disposing==false (as there is no finalizer in this and parent classes) - maybe you just forgot/missed that System.IO.Stream doesn't have a finalizer and it's up to derived class to implement it?

The issue could be completely resolved by adding to DataStream.cs:

~DataStream()
{
     Dispose(false);
}

And GC.SuppressFinalize(this); at the end of the Dispose method.

I could create a pull request if needed. (Created!)

Regards!

KonajuGames commented 6 years ago

System.IO.Stream does call GC.SuppressFinalize(), so that is not required.

aienabled commented 6 years ago

@KonajuGames , thanks! https://referencesource.microsoft.com/#mscorlib/system/io/stream.cs I've looked only on Dispose() and not on the Close() method which is called by Dispose(). So the fix is even simpler - just need a finalizer in the DataStream class.

xoofx commented 6 years ago

Yeah, at that time, DataStream was imported from SlimDX... so I believe it was left as it was exposed there... There are also two other reasons why It has not been updated:

Though PR welcome and going to be merged! 😉