gui-cs / Terminal.Gui

Cross Platform Terminal UI toolkit for .NET
MIT License
9.57k stars 681 forks source link

Need more cases of `ObjectDisposedException` #3675

Open tig opened 3 weeks ago

tig commented 3 weeks ago

I just spent several hours fighting bugs in Menubar and ContextMenu only to discover that they are passing around views (and themsevles) that have been disposed.

We have a lot of this going on and we need to squash it.

This issue is to track adding a bunch of calls like this in key places:

#if DEBUG_IDISPOSABLE
            if (WasDisposed)
            {
                throw new ObjectDisposedException (GetType ().FullName);
            }
#endif
dodexahedron commented 3 weeks ago

Just a reminder:

Don't forget to use the handy-dandy ObjectDisposedException.ThrowIf helper instead of manually constructing and throwing, too!

Better static analysis and optimization, with a little less code, so a win all around!

The code in the initial post would become:

ObjectDisposedException.ThrowIf(WasDisposed,typeof(whateverClassThisIsIn));

Don't use the overload that takes an object instance, as it uses reflection to get the type name (which is all the second argument is for). It calls GetType() at run-time on whatever you passed. typeof(SomeType) is a compile-time constant, which avoids that and is more efficient, to boot. 🥳

dodexahedron commented 3 weeks ago

Also, it's typically not thrown from property accessor methods, unless the accessor itself accesses a different disposable object that has been disposed, and it is supposed to leave the throwing of that up to that object. Basically, it's considered API consumer error, and they're pretty likely to get a NullReferenceException anyway. Plus, guidelines are to avoid throwing any exceptions from property accessors, directly, and that, if you feel the need to, the property should be turned into explicit methods instead.

Methods, however, are expected to throw, ideally with a more detailed InnerException if possible/relevant.

dodexahedron commented 3 weeks ago

A couple of heads ups before you write a whole bunch of code, just in case I didn't beat this particular horse enough or properly last time we did exception work (plus I'm pretty sure I learned some nuance already included in the above since then, as well). 😄

dodexahedron commented 3 weeks ago

Oooo

And for methods that return IDisposables, and any types that are themselves IDisposable, it's friendly to include the [MustDisposeResource(bool)] attribute, with true if the caller is expected to dispose it or false if it doesn't matter who disposes it.

tig commented 3 weeks ago

Here's the issue I found that led to this exercise:

https://github.com/gui-cs/Terminal.Gui/issues/3678