CODEWITHSUNDEEP
.net
kkirkfield
kkirkfield

Reputation: 85

.Net Framework Bug: System.Net.Mail.AttachmentBase disposes of streams it did't create

Before submitting this as a bug, I wanted to get community feedback to see if this should be considered a bug.

When creating a LinkedResource or other class that derives from AttachmentBase, there are constructor overloads that take either a file name or a Stream. When given a file name, AttachmentBase internally creates a FileStream and assigns it the private MimePart part content using MimePart.SetContent(Stream). When creating an AttachmentBase using the constructor that takes a Stream, it directly assigns the underlying content to this Stream.

When the AttachmentBase is disposed, it internally disposes of the MimePart, which internally disposes of the Stream content. If the constructor taking a Stream was used, this MimePart class just disposed of a Stream it did not create! This bug is a side effect. Disposing of objects that a class/method doesn't own makes it impossible to reuse that object later. When creating a disposable object, that object should be able to be disposed before it is garbage collected. This allows it to free up resources like file handles captured by FileStreams.

Suppose you need to send out a lot of emails, and each email has one image. The image is very large, and all the emails use this same exact image. You should be able to read this file into a MemoryStream, and reuse this Stream for the LinkedResource in each email. This would allow little disk read and low memory usage. Dispose() is invoked on each message and its resources to free up any handles and unmanaged memory once the message is sent. (Again, if it implements IDisposable, it should be able to be disposed as soon as it's no longer needed. The code that called Dispose() should always be the same code that created the object.) Because this Dispose() method is not implemented correctly, it just disposed the underlying Stream to the image that it did not create. When subsequent messages try to use this Stream, they will throw an ObjectDisposedException.

A temporary workaround for this bug is to copy the MemoryStream for each message, and use the copied Streams. This still results in little disk read, but now the large image is copied a bunch of times in memory.

This could have all been avoided by following correct patterns and practices and allowing the code that created the Stream to be the one to dispose of it, and not assuming it would only be used once. Here is a fix for this bug. The code should replace the similar methods. This allows MimePart to be backward compatible. The change to AttachmentBase may be a breaking change for code that passes in a Stream. The fix is to handle the Stream you pass in by disposing of it yourself properly.

public abstract class AttachmentBase : IDisposable
{
    MimePart part;

    protected AttachmentBase(Stream contentStream)
    {
        part.SetContent(contentStream, true);
    }

    protected AttachmentBase(Stream contentStream, string mediaType)
    {
        part.SetContent(contentStream, null, mediaType, true);
    }

    internal AttachmentBase(Stream contentStream, string name, string mediaType)
    {
        part.SetContent(contentStream, name, mediaType, true);
    }

    protected AttachmentBase(Stream contentStream, ContentType contentType)
    {
        part.SetContent(contentStream, contentType, true);
    }
}

internal class MimePart : MimeBasePart, IDisposable
{
    private bool _keepOpen;

    internal void SetContent(Stream stream, bool keepOpen = false)
    {
        if (stream == null)
        {
            throw new ArgumentNullException("stream");
        }

        if (streamSet && !_keepOpen)
        {
            this.stream.Dispose();
        }

        this.stream = stream;
        streamSet = true;
        streamUsedOnce = false;
        TransferEncoding = TransferEncoding.Base64;
        _keepOpen = keepOpen;
    }

    internal void SetContent(Stream stream, string name, string mimeType, bool keepOpen = false)
    {
        if (stream == null)
        {
            throw new ArgumentNullException("stream");
        }

        if (!string.IsNullOrEmpty(mimeType))
        {
            contentType = new ContentType(mimeType);
        }

        if (!string.IsNullOrEmpty(name))
        {
            ContentType.Name = name;
        }

        SetContent(stream, keepOpen);
    }

    internal void SetContent(Stream stream, ContentType contentType, bool keepOpen = false)
    {
        if (stream == null)
        {
            throw new ArgumentNullException("stream");
        }

        this.contentType = contentType;
        SetContent(stream, keepOpen);
    }

    public void Dispose()
    {
        if (stream != null && !_keepOpen)
        {
            stream.Dispose();
        }
    }
}

Upvotes: 1

Views: 123

Answers (1)

John Koerner
John Koerner

Reputation: 38077

I think I agree that the KeepOpen option should be something supported by this API, similar to what the StreamReader does. However, your suggested implementation would probably have to include it all the way back to where the consumer creates the objects, to prevent breaking changes from being introduced.

Another way to workaround this issue is to create a wrapping stream that ignores calls to close and having an alternative way to close the stream when you are done.

For example:

private class UncloseableStreamWrapper : Stream
{
    private readonly Stream _baseStream;
    public UncloseableStreamWrapper(Stream baseStream)
    {
        _baseStream = baseStream;
    }

    public override void Close()
    {
        // DO NOTHING HERE
    }

    public void CloseWrappedStream()
    {
        _baseStream.Close();
    }
    public override long Position
    {
        get
        {
            return _baseStream.Position;
        }
        set
        {
            _baseStream.Position = value;
        }
    }

    public override bool CanRead => _baseStream.CanRead;
    public override bool CanSeek => _baseStream.CanSeek;
    public override bool CanWrite => _baseStream.CanWrite;
    public override long Length => _baseStream.Length;
    public override void Flush() => _baseStream.Flush();
    public override int Read(byte[] buffer, int offset, int count)=> _baseStream.Read(buffer, offset, count);
    public override long Seek(long offset, SeekOrigin origin)=>_baseStream.Seek(offset, origin);
    public override void SetLength(long value) => _baseStream.SetLength(value);
    public override void Write(byte[] buffer, int offset, int count) => _baseStream.Write(buffer, offset, count);
}

Then in your loop, you can just reset the position and go.

var stream = new UncloseableStreamWrapper(System.IO.File.OpenRead(@"Z:\temp\temp.png"));
var lr = new LinkedResource(stream);
lr.Dispose();

stream.Position = 0;

lr = new LinkedResource(stream);
lr.Dispose();

stream.CloseWrappedStream();
Console.ReadLine();

One big caveat here is that you are tying your code to the implementation of the underlying MimePart which could change with a future update that would completely break your code.

Upvotes: 2

Related Questions