Tutorial: Questionable Shader class disposal advice/code

[MV10]

I had copied the Shader class without initially thinking about it much. Today I was basically rewriting it and noticed the disposal logic, much of which originated with the tutorial code. This is a bit nitpicky, but the discussion of unmanaged resource cleanup in the 1.2 Hello Triangle chapter at the end of the Compiling the Shaders section may warrant reconsideration.

.NET finalizers aren't remotely equivalent to C++ destructors, so it doesn't make sense to state, "We can't do it in the finalizer due to the Object-Oriented Language Problem." Nobody should be contemplating resource cleanup in the finalizer as correct behavior in the first place, completely irrespective of whether or not it's problematic for OpenGL specifically.

The code, of course, is meant to output a warning if disposal wasn't correctly called but (a) that's detrimental to performance because declaring a finalizer adds it to .NET's finalizer tracking queue, (b) there's no guarantee about when a finalizer might be invoked, or whether it will be invoked at all, ever, and (c) in practice they are never invoked when the application is exiting, which is the most likely scenario here. So in short, while a warning message looks like a good idea, in reality it'll never actually be seen. It's wasted effort.

I'd say it makes the most sense just to show a Dispose method. Also, I suppose the sample code was cut-and-pasted from somewhere else since arguments like bool disposing aren't even used (that's an arguably-bad design when you want to know whether or not Dispose is being called from the finalizer.) Even though the class shouldn't have a finalizer, it should still call SuppressFinalize in case a derived class does declare a finalizer.

Just keep it simple:

private bool IsDisposed = false;

public virtual void Dispose()
{
    if (IsDisposed) return;

    GL.DeleteProgram(Handle);
    IsDisposed = true;
    GC.SuppressFinalize(this);
}

Like I said, nitpicky.

[deccer]

Simply seal the class and remove the GC.XXX. There is virtually - no pun intended - no need to derive from Shader in the first place, but rather inject it or instantiate it in place

[MV10]

@deccer I disagree. For example, my user-facing app wraps the shader class from my library to provide additional context, like the key value used by an LRU caching scheme. DI isn't practical in my case since the program is almost constantly loading arbitrary combinations of shader programs and libraries.

Sealing would be a pretty drastic design decision just to avoid issuing SuppressFinalize. Technically if you're really opposed to the GC command for some reason, omitting it wouldn't really hurt anything, and doesn't change the points I made: the docs shouldn't mention finalizers and the sample code isn't handling disposal properly.

[NogginBops]

@MV10

I agree that quoting the "Object Oriented Problem" is misleading here (we should be talking about the fact that finalizers run on their own thread and can't call gl because of that). But the shader class implementation is actually fine.

I will address the three main concerns.

(a) that's detrimental to performance because declaring a finalizer adds it to .NET's finalizer tracking queue

This is negligible and if you dispose the object properly it never gets added to the finalize queue anyway.

(b) there's no guarantee about when a finalizer might be invoked, or whether it will be invoked at all

This is not a problem as this is not meant to be a 100% bullet proof solution. It's meant to indicate and remind you that you might be leaking OpenGL objects during runtime.

(c) in practice they are never invoked when the application is exiting, which is the most likely scenario here.

This is by design. You should NOT be cleaning up OpenGL objects at program termination. The operating system and gpu driver are infinitely faster at nuking all of your allocated memory than you having to do it manually yourself. You are just making termination of your program needlessly slow.

The whole bool isDisposing business comes from the Microsoft official guidelines for implementing IDisposable.

When I have time I will revise this chapter of the tutorial and fix some of the issues and do a little cleanup.

Also a reminder that the tutorial code in no way aims to be "production ready" code. It's meant for learning and clarity, not for robust and scalable implementation. The tutorial code should be modified heavily if used as a base for a game project.