Stream closed automatically on MailMessage.Save(stream)

[cgallegu]

In https://github.com/andyedinborough/aenetmail/blob/25d0253d6a597163e14909dfceeafdfa8bda7afd/MailMessage.cs#L219, stream is being closed automatically by the using clause. That prevents from using the stream for other things (for example, seeking). Is this the intended behavior?

[andyedinborough]

Good catch. It should probably be updated as:

public virtual void Save(System.IO.Stream stream, Encoding encoding = null) {
    var str = new System.IO.StreamWriter(stream, encoding ?? System.Text.Encoding.Default))
    Save(str);
    str.Flush();
}

Thoughts?

[jstedfast]

The GC will still collect 'str' at some point, all you've changed is the certainty of when 'str' will get disposed.

I'd say your change makes the problem worse.

[andyedinborough]

Of course it will. That's not the point.

The point is that this method shouldn't be the one to dispose the stream. That should be up to the client.

[cgallegu]

Yes, that's the pattern I've seen when using streams. It makes sense when you need to do something with the stream later.

Of course it will. That's not the point.

The point is that this method shouldn't be the one to dispose the stream. That should be up to the client.

— Reply to this email directly or view it on GitHub https://github.com/andyedinborough/aenetmail/issues/163#issuecomment-47469436 .

[jstedfast]

My point is that for all you know, the GC might still dispose the str (which will dispose the stream) when the method exits - but worse than that, it might only do it some times and not others.

In other words, sometimes it might be safe to use 'stream' after calling the Save() method and sometimes it will randomly fail due to the finalizer thread just happening to decide to GC right then.

A third explanation of the problem your change creates is that all you've done is introduce a race condition. As you probably know, the GC's finalizer runs in its own thread and it can collect objects that nothing is pointing to AT ANY TIME. Once the Save() method exits, nothing is pointing to 'str' - hence the GC can collect it at any time. It can collect it the microsecond that the method exits or it could collect it 10 minutes afterward. You don't know when it'll happen. This means that using 'stream' after the Save() method exits is a crap-shoot - you never know if it'll be disposed or not.

[jstedfast]

I've just checked the StreamWriter docs and unfortunately there does not appear to be a StreamWriter constructor that takes a 'leaveOpen' argument (if there was, it would be the perfect solution for this problem).

You have 2 options for reliably fixing this problem:

1. Implement your own StreamWriter (subclassing from TextWriter) that does not close or dispose the stream when it gets disposed.
2. Change the API to take a StreamWriter argument so that you do not need to create your own.

[jstedfast]

.NET 4.5 has:

public StreamWriter (Stream stream, Encoding encoding, int bufferSize, bool leaveOpen)

[andyedinborough]

Thanks, I'll implement the leaveOpen constructor for the .NET 4.5 version.

However, the original reason I wrote it this way is still valid. str is dereferenced once this method has finished executing and will eventually be picked up by the GC, but the finalizer will not dispose or close the stream. Finalizers won't dispose other objects.

http://msdn.microsoft.com/en-us/library/b1yfkh5e(v=vs.110).aspx

[andyedinborough]

As you can see from Mono's implementation, StreamWriter follows the pattern and does not dispose the stream when being finalized:

https://github.com/mono/mono/blob/master/mcs/class/corlib/System.IO/StreamWriter.cs#L172

[jstedfast]

Yep, looks like you are correct. Sorry about that.