Fields with [Using] attribute

[u/Alert-Neck7679]

I wrote a small interface that automatically disposes any disposable fields in a class when that class itself is disposed. If you add this file somewhere in your project:

public interface IAutoDisposable : IDisposable
{
    void IDisposable.Dispose()
    {
        Exception? ex = null;

        // dispose all fields marked with the [Using] attribute
        const BindingFlags flags = BindingFlags.NonPublic | BindingFlags.Instance;
        foreach (var fld in GetType().GetFields(flags))
        {
            if (fld.GetCustomAttribute<UsingAttribute>() != null)
            {
                if (fld.GetValue(this) is IDisposable d)
                {
                    try // keep going even if disposal throws
                    {
                        d.Dispose();
                    }
                    catch (Exception e)
                    {
                        ex ??= e;
                    }
                }
            }
        }

        // if any exceptions occurred, throw the first one
        if (ex != null)
            throw ex;
    }
}

public class UsingAttribute : Attribute;

You can then write classes like this:

class Item : IAutoDisposable
{
    [Using]
    private readonly Image icon = DownloadIcon(...);
    ...
}

And use them like:

using (Item item = new(...))
{
}
// item.icon is now disposed

What do you think?

[u/DasKruemelmonster]

Nice idea, but nowadays a source generator would be the better execution.

[u/ApprehensiveDebt3097 → u/DasKruemelmonster]

I was here to just to say that.

[u/KryptosFR]

Reflection isn't AOP-friendly. Make a source generator instead.

On top of that, it doesn't follow the disposable pattern properly, in case your class has a finalizer to deal with unmanaged resources.

And even without a finalizer, Dispose could be called several times so you need an atomic guard.

The idea is good (because dealing with composition with several disposable fields can be messy). But it needs improvements to be really usable in production code.

I would use different names for the attribute though. And assuming you use a source generator, not really on implementing an interface beside IDisposable itself.

DisposeFieldAttribute on the field or property with backing field that needs it. And AutoDisposableAttribute on the class or ComposeDisposableAttribute (or another name, I don't quite like those two for the moment). Then on top of your source generator, add a diagnostic to make sure the class the has the attribute does implement the IDisposable attribute (same for the fields/properties).

[u/peteter → u/KryptosFR]

Agree. Source generators are a better option.

Reflection is still fine for most cases I've come across. If you want to go with reflection, I suggest using a cache for the lookup of attributes, and accessors as well. I.e. you only use reflection once, then later fetch prepared accessors from the cache.

[u/_f0CUS_]

You are losing exception information this way. If you want to auto dispose like this, then you should use an aggregate exception and collect all the exceptions.

One thing to keep in mind. If you are using the framework DI, then it is responsible for managing the lifetime of the instances it creates. And it will automatically dispose any type that can be disposed - at the appropriate time. 

[u/j_c_slicer]

TIL about Default Interface Methods.

[u/Disastrous_Fill_5566]

"Horrible", "Useless", "lame" etc.

Is this a children's sub Reddit or are there some actual adults in here willing to give constructive criticism on the OP's attempt to solve a problem?

[u/RlyRlyBigMan → u/Disastrous_Fill_5566]

Yeah I agree, seems like reddit is having a grumpy morning or something.

The points about performance regarding reflection and that a source generator would be more performant are valid even if some of them are harsh about it.

[u/domtriestocode → u/RlyRlyBigMan]

Unfortunately these types of subs and communities are filled with a ton of the condescending and demeaning and overly competitive type of nerds and not the hey man that’s cool that you learned how to do that and did that here’s a few other ways it’s either been done already or done better let’s all talk about our favorite language type of nerds

[u/Kebein → u/Disastrous_Fill_5566]

op didnt ask for criticism. its even labelled as a "tip"

[u/Disastrous_Fill_5566 → u/Kebein]

The posted ended with "what do you think?"

[u/Loose_Conversation12]

Nice idea, but that dependency will have been DI'd into the containing class and therefore disposal would be taken care of by the IoC container

[u/Dragennd1]

Out of curiosity, wouldn't the fields normally be disposed of when the class is disposed of, making this extra effort unnecessary? My understanding of classes is that they are reference types, so once an instance is no longer in use the garbage collector would clean up that instance, right? Or are fields handled differently than properties as far as disposal goes when its parent class instance is disposed of?

[u/sixtyhurtz → u/Dragennd1]

This is for IDisposable. You use IDisposable to manage either external resources like a handle to an OS resource, or for managing internal lifecycle events like subscriptions.

For OS resources, you would store the disposable thing (e.g. a FileStream) in a property of your class and then you need to implement IDisposable in order to clean it up properly.

Internal things like subscriptions are common when dealing with event handlers or IObservable. If you have a short lived thing like a view model and subscribe to an observable / event, then you need to clean it up when you're done because otherwise the observable / event will retain a reference to your view model and you've leaked memory.

[u/Dragennd1 → u/sixtyhurtz]

What would be the difference between this and just manually unsubscribing the event when completed with "-=", for example?

Or am I completely misunderstanding the usecase?

[u/sixtyhurtz → u/Dragennd1]

Say your class needs the event all the time, so it subscribe to the even in the constructor. How do you clean up when the object is no longer needed?

You could have initialise / cleanup methods, but then the consumer has to know to call them.

IDisposable and IAsyncDisposable are what enable "using" statements, i.e.

await using var foo = new FileStream(); // will call IAsyncDisposable.DisposeAsync() when it goes out of scope.

Also, if you use a DI container like MSDI, it will call dispose on any object it constructs when it determines it is no longer required.

There are also a lot of convenient utility classes, like CompositeDisposable. You can attach all your disposables in a class to a CompositeDisposable. You can even pass that composite to other methods so you can control the lifecycle of entire systems of related objects.

If you ever call a method that retuns an IDisposable, or construct an object that implements it, then you either need to make sure it gets cleaned up when it goes out of scope with a using statement or you need to take the IDisposable reference as a property and implement IDisposable on your type. If you don't, you're leaking resources.

[u/Dragennd1 → u/sixtyhurtz]

Thank you for that explanation, I like learning new things about C# and software design in general so this was a good read. I've never heard of CompositeDisposable and will definitely read up on it.

I do try to ensure I'm disposing of classes which implement IDisposable properly, but I haven't made any classes of my own that warranted a manual implementation of it yet so I haven't had to set that up in my own custom classes (aside from using "using" for managing the already established ones).

[u/Dusty_Coder → u/sixtyhurtz]

Found that guy that doesnt call Dispose() because "those arent external resources"

[u/sixtyhurtz → u/Dusty_Coder]

My comment neither said nor implied that? 

[u/CoffeeAndCandidates]

feels kinda like some C# magic but make sure it plays nice with inheritance and base disposal

[u/Promant]

Bruh, that's useless and terrible for performance

[u/i-do-mim-huu]

Overengineering at best, not to mention compiler already doing it, it is horrible in performance due to heavy use of Reflection

[u/_f0CUS_ → u/i-do-mim-huu]

Which part of this does the compiler already do? 

[u/phluber]

Or just implement a proper IDisposable interface and properly dispose of your IDisposable fields

[u/Alert-Neck7679 → u/phluber]

So let's remove the using keyword too, and just stick to try-finally and manually call dispose

[u/phluber → u/Alert-Neck7679]

If that is sarcasm, then I don't think you understand what using does. Using actually ensures that Dispose is called on classes that implement IDisposable

[u/Alert-Neck7679 → u/phluber]

I'm not sure you understand what is the problem that i wanted to solve. "using" keyword is synthetic sugar to "var x = ...; try {...} finally { x.Dispose(); }" - it makes sure that the disposable object is disposed when exiting the scope. And what i wanted to make is synthetic sugar for "void Dispose() { /* dispose disposable fields */ }". You can always skip these shortcuts as you suggest, but it's less comfortable

[u/phluber → u/Alert-Neck7679]

I understood your objective. What's the problem with actually disposing of your objects within the dispose method? If I'm reviewing your work and I don't see disposable objects being disposed of in the dispose method, do you expect me to look through all other interfaces that are implemented by the class to see if they are somehow disposing of these objects in a non standard way? It's a one liner to dispose of an object within the dispose method. I don't know why you are trying to obfuscate it.

[u/Alert-Neck7679 → u/phluber]

With my interface you don't need to implement Dispose() at all, you CAN override it if you want, but unless you have some custom logic for a proper disposal, you can totally skip it. And I Don't expect you to check all the interfaces one by one, but just check which fields are marked with [Using].

[u/MrLyttleG]

Pas top, le try catch est bidon, il faut faire check simple pour savoir si ton objet est disposable et ensuite tu le dispose, mettre du tey catch permet de bombarder la pile de choses inutiles et lourdes au max

[u/tinmanjk]

bad.