Implementing IDisposable on MimeEntity

[polterguy]

Is your feature request related to a problem? Please describe.
When I create a MimeEntity that somehow loads content from files, this is typically done by opening stream and associating these with the Content object. This implies I'll have to manually track all streams associated directly and indirectly with my MimeEntity, which of course creates more complex and less "DRY" code for me.

Describe the solution you'd like
Basically, implement IDisposable interface on MimeEntity, for then to have its implementation recursively disposing all Stream objects directly or indirectly associated with the MimeEntity. Below is example code.

// Invoked from MimeEntity.Dispose
static void DisposeEntity(MimeEntity entity)
{
    if (entity is MimePart part)
    {
        part.Content?.Stream?.Dispose();
    }
    else if (entity is Multipart multi)
    {
        foreach (var idx in multi)
        {
            DisposeEntity(idx);
        }
    }
}

Describe alternatives you've considered
The alternative is to duplicate the above code every time I might in theory have an open Stream somehow associated with my MimeEntity, which of course is sub-optimal.

[jstedfast]

I had a patch for this here: https://github.com/jstedfast/MimeKit/blob/master/disposable.patch

It probably doesn't apply cleanly anymore, but it's a start.

IIRC, I did not commit the patch because it caused the unit tests to fail and I never figured out how/why.

[polterguy]

Weird, if you've got a non-disposable class, obviously none of the tests invokes Dispose I assume ... :/
Maybe some dependency injection parts, maybe in the tests, that would explain why Dispose is invoked ...?

Do you mind if I have a go at it, or ...?

[jstedfast]

I just updated disposable.patch. Feel free to play around with it.

[jstedfast]

As I ported the patch, I spotted the issues.

[polterguy]

Does this imply the next release will have IDisposable on MimeEntity ...?

[jstedfast]

Depends...

Keep in mind that things get more complicated when you add IDisposable support.

Should multipart[i] = newPart dispose the old part at that location? What about multipart.Remove(part)? What are the implications of these changes?

[polterguy]

Should multipart[i] = newPart dispose

Not really, although I see your point, this is a "C++ way of thinking". The important part is to have MimeEntity cleanup automatically without having to add code to specifically do it in client code.

MIME is a tree structure, implying people typically open Stream objects recursively during construction of entities. It's a question of "ownership". The default behaviour should be that MimeEntity should "own" the stream, the same way a StreamReader owns its Stream and disposes it. This is the default behaviour for most classes that is using IDisposable instance as fields, either directly or indirectly.

Then if some don't like this behaviour, they can simply avoid invoking Dispose ...

I cannot imagine how this would break existing code, except in cases where others have inherited from MimeEntity and created their own method and named it Dispose - However, even these cases can be avoided by implementing IDisposable explicitly ...

[jstedfast]

It would seem intuitive to me that the following multipart.Clear () and multipart[i] = newPart would both dispose the old entities.

Those types of things are the types of things that broke my unit tests, btw.

[polterguy]

Hmm, I had an idea as I started thinking about our little problem. Maybe just keep the existing classes as they are, and create "YALOA" ...? (Yet Another Layer Of Abstraction)

As in, create a class inheriting from MimeEntity called e.g. MimeEntityDisposable (or something) that implements IDisposable, and recursively clean up things ...?

That way there would be zero changes to the existing classes, yet still allow us to consume the library "as is" and easily clean up stuff ...?

Not sure how that would work in regards to MailKit, and places that creates MimeEntity instances though, which might be an issue ...?

My current problem is that I've got to add "clean up code" every place I create MimeEntity instances, which implies having to either duplicate code in multiple projects, and/or create a YALOA project myself, complicating consumption of MimeKit ... :/

Those types of things are the types of things that broke my unit tests, btw

OK, so you disposed old entities at the above points ...?
Does it really need to?
If MimeEntity implements IDisposable then oldPart would be a MimeEntity I presume, and one could do as follows ...

var x = multipart[i];
x.Dispose();
multipart[i] = newPart;

...?

Even if that's bad, the MimeEntityDisposable class could override Clear and the index operator I presume ...?

Maybe this becomes dirty'ish, but a lot of people would probably use middleware code, created by layers between MimeKit and client/application code, and the first thing they'd look for, is if MimeEntity implements IDisposable, and if not, they would imagine everything is OK, when it's actually not, and they'd be leaking stream objects, etc, etc, etc ...

[jstedfast]

Why wouldn't you just implement an extension method?

public static void Dispose (this MimeEntity entity)
{
    if (entity is Multipart multipart) {
        for (int i = 0; i < multipart.Count; i++)
            multipart[i].Dispose ();
    } else if (entity is MessagePart rfc822) {
        if (rfc822.Message != null)
            rfc822.Message.Dispose ();
    } else if (entity is MimePart part) {
        if (part.Content != null)
            part.Content.Stream.Dispose ();
    }
}

public static void Dispose (this MimeMessage message)
{
    message.Body.Dispose ();
}

[polterguy]

I'd need to duplicate code in all projects referencing the project, but don't worry, I'll figure it out ...

[jstedfast]

Decided the best coarse would be to make Remove/RemoveAt/Clear/set_Item not dispose.

Added a Clear(bool dispose) method for convenience for the Clear() case.

[polterguy]

Thank you man, this makes my life a 100 times easier :)
I'm using MimeKit "all over the place" and having this results in my code becoming much more DRY ... :)

Let me know when you've got a release out somehow with this. Maybe just commenting "done" on this guy or something.

[jstedfast]

MimeKit v3.0.0 has been released with this feature.

[polterguy]

Najs, thx man :)

[polterguy]

Sorry to bother you, but for consistency reasons we should probably implement IDisposable on MailMessage too. It's not a big deal for me, but I thought it might create more consistency for (other) users ...

Thank you for the MimeEntity changes, it allowed me to remove 3 duplicated methods in 3 different projects ... :)

[jstedfast]

You mean MimeMessage? (MailMessage is System.Net.Mail)

If so, I already made MimeMessage implement IDisposable.

[polterguy]

Thx mate :)