13

So, after discovering that the Bitmap class expects the original stream to stay open for the life of the image or bitmap, I decided to find out if the Bitmap class actually closes the stream when it is disposed.

Looking at the source code, the Bitmap and Image classes create a GPStream instance to wrap the stream, but do not store a reference to either the GPStream or the Stream instance.

num = SafeNativeMethods.Gdip.GdipLoadImageFromStreamICM(new GPStream(stream), out zero);

Now, the GPStream class (internal), does not implement a Release or Dispose method - nothing that would allow GDI to close or dispose of the stream. And since the Image/Bitmap class doesn't keep a reference to the GPStream instance, it seems that there is absolutely no way for either GDI, Drawing.Bitmap, or Drawing.Stream to close the stream properly.

I could subclass Bitmap to fix this, but, oh wait, it's sealed.

Please tell me I'm wrong, and that MS didn't just make it impossible to write code that doesn't leak resources with their API.

Keep in mind (a), Bitmap has no managed reference to the stream, meaning GC will collect it while it is still in use, and (b) .NET APIs take Bitmap/Image references and aren't deterministic about when they're done with them.

Community
  • 1
  • 1
Lilith River
  • 16,204
  • 2
  • 44
  • 76
  • 1
    Yeah... ever since I stumbled on this mess I just deep-clone all images I open using `LockBits` and `Marshal.Copy`. Starting from a `Bitmap` object created without any linked resources is the only way to be sure. – Nyerguds Jan 28 '18 at 00:34
  • @Nyerguds that's actually a nice solution. However, At this point I'd recommend not using System.Drawing API when possible. It's just a pain in the ass, specially regarding object lifetime management and leaks are so easy to accidentally create. – AsPas Jan 24 '21 at 11:22
  • @AsPas well, object management is just a mindset. It's perfectly possible to be meticulous enough in that to never have memory leaks. Languages like C++ have nothing like automatic garbage collection anyway. But yea, `System.Drawing` is kind of showing its age these days, and is very much not universally usable. – Nyerguds Jan 24 '21 at 14:23
  • I wrote Imageflow.NET to avoid using System.Drawing. See https://github.com/imazen/imageflow-dotnet – Lilith River Jan 25 '21 at 14:57
  • @LilithRiver well, if you're using `Windows.Forms` and want to display images in common Windows controls, you're still stuck with `System.Drawing` anyway... – Nyerguds Feb 16 '21 at 09:34

4 Answers4

9

Since you supply the stream in this example, I'd imagine you are responsible for disposing it.

C.Evenhuis
  • 25,996
  • 2
  • 58
  • 72
  • Exactly. This has been discussed before - Bitmap has no responsibility for the lifetime of the stream. What if you wanted to do something else with the same stream after playing with Bitmap? You'd be very disappointed to learn Bitmap disposed it for you. – n8wrl May 25 '11 at 11:25
  • Yes, if the constructor was finished with the stream. But when an object 'takes' a stream, and uses it at arbitrary times, ownership is transferred. In this case, only the Bitmap class knows when it is 'done' with the stream, and so only the Bitmap class can be responsible for closing it. – Lilith River May 25 '11 at 11:27
  • Usually an object would make a reference of its own to the stream, keeping it from being garbage collected for as long as it's needed - but not disposing it, as it would result in unreadable code and perhaps even untraceable "happens once in a blue moon" bugs. – C.Evenhuis May 25 '11 at 11:46
  • Ideally, it would read everything into memory and you could dispose the stream immediately after it was read. Apparently it keeps the stream open *to read metadata* on demand. Pretty poor excuse. – Lilith River May 25 '11 at 13:44
  • 1
    It also fails to keep a reference to the stream, so the stream gets garbage collected while it's still in use (since the reference is passed back through COM). There have been SO posts about the problem. – Lilith River May 25 '11 at 13:46
  • Bitmap is a documented exception to the rule, and perhaps an unnecessary one - I don't disagree with that. I do disagree though with wanting Bitmap to dispose the stream, making it an even worse object to work with. – C.Evenhuis May 25 '11 at 14:27
  • 2
    Well, if either (a) it could load it 100% into memory instead of just 99% into memory, (b) not be *sealed*, (c) offer a disposing event, (d) have a boolean for closing the stream on dispose, or (e) simply keep a stinkin' reference to stream so GC works properly, it would be much easier to work with. – Lilith River May 25 '11 at 15:36
3

It is a good practice to have the method that opens a stream, close it as well. That way it is easier to keep track of leaks. It would be quite strange to have an other object closing the stream that you opened.

Emond
  • 50,210
  • 11
  • 84
  • 115
  • 5
    Passing around both a Bitmap and an (optional) stream instance is rather painful. Looking at all of the APIs I've seen, *nobody* expects this behavior. – Lilith River May 25 '11 at 11:31
  • @ComputerLinguist - If you feel the need to pass these two around your design might be wrong. In such a case I would try to make an object (not a single method) responsible for opening and closing the stream. – Emond Jul 27 '13 at 05:42
  • Consider that Streams themselves can be configured to close the parent stream, and that Bitmap itself has streaming behavior. – Lilith River Jul 27 '13 at 15:53
0

Because bitmap can't guarantee in which order the destructor is called it will not close the stream because it may already have been closed with its own destructor during garbage collection. Jeffrey Richter's CLR via C# has a chapter on memory management that explains with much more clarity than I can.

Ryan
  • 1
  • 3
    Actually, it can. Bitmap will throw random COM exceptions if you close the underlying stream before disposing of the Bitmap. Pass a MemoryStream to a new Bitmap instance, wait a bit, access bitmap.Width, and watch the exceptions fly. – Lilith River Jul 27 '13 at 15:55
0

An easy workaround to the problem is:

var image = new Bitmap(stream);
image.Tag = stream;

Now the stream is referenced by the image and won't be garbage collected before the image is. If your stream happens to be a MemoryStream, it doesn't need to be disposed (its Dispose is a no-op). If not, you can dispose it when you dispose the image, or just let the GC do it when it gets around to it.

d ei
  • 493
  • 4
  • 16
  • I've used this in the past, and I remember it working. That said, you definitely don't want to wait for the GC to clean up an open file handle, that's gonna cause all kinds of pain. – Lilith River Jan 28 '22 at 21:36