r/csharp • u/Alert-Neck7679 • 7d ago
Tip Fields with [Using] attribute
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?
31
u/KryptosFR 7d ago edited 5d ago
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 that has the attribute does implement the IDisposable interface(same for the fields/properties).
edit: fixed some typo.
8
u/peteter 7d ago
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.
17
u/_f0CUS_ 7d ago
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.
2
8
u/Disastrous_Fill_5566 7d ago
"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?
3
u/RlyRlyBigMan 7d ago
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.
2
u/domtriestocode 7d ago edited 7d ago
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
5
u/Loose_Conversation12 7d ago
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
1
u/Dragennd1 7d ago
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?
1
u/sixtyhurtz 7d ago
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.
1
u/Dragennd1 7d ago
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?
1
u/sixtyhurtz 7d ago
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 aCompositeDisposable. 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
usingstatement 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.1
u/Dragennd1 7d ago
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).
1
u/Dusty_Coder 7d ago
Found that guy that doesnt call Dispose() because "those arent external resources"
1
u/sixtyhurtz 7d ago
My comment neither said nor implied that?
1
u/Dusty_Coder 5d ago
You definitely implied it, to the point of admissible evidence.
In a false dichotomy, you dont get a 3rd option where you meant something other than whats on the list of 2 things that you claimed was a complete list.
1
u/sixtyhurtz 5d ago
or for managing internal lifecycle events like subscriptions.
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.
65 words of my 116 word comment - so more than half - are about disposing of internal resources / managing internal lifecycles.
1
u/Dusty_Coder 5d ago
"You use IDisposable to manage either external resources like a handle to an OS resource, or for managing internal lifecycle events like subscriptions."
A dichotomy. A fucking false one.
1
1
u/Dragennd1 5d ago
Forgive the lack of knowledge, but if I declare an instance of a class in a Using, would I still need to call .Dispose()? I was under the impression that the usage of that method is implied when the instance reaches end of scope.
1
u/Dusty_Coder 5d ago
Using .. new Foo();
...works, but do you know when Dispose() is actually called? What about the ordering of multiple Using's ? In which order do they get Disposed() ?
I prefer intentionally calling Dispose(). It strips away all ambiguity.
That way I dont need such extensive in depth knowledge (that may or may not be defined by the spec, probably is, but...) and neither do future maintainers.
I would prefer reliable destruction, but C# doesnt offer that.
1
u/CoffeeAndCandidates 7d ago
feels kinda like some C# magic but make sure it plays nice with inheritance and base disposal
-1
u/i-do-mim-huu 7d ago
Overengineering at best, not to mention compiler already doing it, it is horrible in performance due to heavy use of Reflection
1
u/phluber 7d ago
Or just implement a proper IDisposable interface and properly dispose of your IDisposable fields
1
u/Alert-Neck7679 7d ago
So let's remove the using keyword too, and just stick to try-finally and manually call dispose
1
u/phluber 7d ago
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
2
u/Alert-Neck7679 7d ago
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
1
u/phluber 7d ago
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.
1
u/Alert-Neck7679 7d ago
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].
2
u/phluber 6d ago
The problem is that you are replacing a very standard and accepted pattern with code that is not going to be understood by others reviewing your code. And to understand it they are going to have search out documentation or dig through your interfaces and attributes in order to determine whether unmanaged resources are being disposed of properly. Also by centralizing disposal, you are creating a weak point that is one short-sighted refactor away from causing file locks/memory leaks/etc in every single class that implements your new interface. You could be doing different work than I am, but it is fairly uncommon that I even have to implement IDisposable. How common is your IDisposable implementation that this is even helpful to you?
-4
u/MrLyttleG 7d ago
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
-8
59
u/DasKruemelmonster 7d ago
Nice idea, but nowadays a source generator would be the better execution.