3

How do you make an class that properly warns a developer in the future that they've made a mistake somewhere in their implementation that resulted in an object that gets deconstructed in a state that prevents the release of it's resources?

Background:

I recently upgraded to Visual Studio 2015 and began reloading and compiling code for a game engine I'm working on and ran into a new series of warnings "warning C4297: '*': function assumed not to throw an exception but does". A quick search revealed a C++ convention that I'd missed and the reasons behind said convention: destructors should not throw exceptions. I also can't really argue with the reasons, but I'm also not sure how to work around the problem.

Within OpenGL a Context basically holds all of the state information for the OpenGL engine. Only one thread may have a context at any given time and each thread may only have one current context. When the engine starts it creates the context and then relinquishes control over the context and starts up another thread which picks it up and proceeds to handle the graphics rendering for the engine. To handle all of this, I created a graphics engine class that uses semantics similar to a mutex to claim and relinquish the graphics engine and make sure that no mistakes are made that might some day result in someone attempting to do things with a context that it doesn't own.

During destruction, the graphics engine and a number of other classes that rely on it all check to make sure that the current thread has claimed the graphics engine before they perform actions that are necessary in their destruction. If the thread didn't have the graphics context claimed, the destructor was throwing. My goal was really to provide some basic protection against the class being used improperly on accident in the future, not to make the graphics engine thread-safe. Now... I'm uncertain of how best to handle this.

I've contemplated just switching over to a mutex-based approach which I could use to block access to the graphics context until a thread was done, possibly making the graphics engine class fully multi-threading capable (not that I can understand why you'd want to perform multi-threading with an OpenGL context, as the calls needed to do so are expensive enough to negate any benefit you might get out of it from what I understand).

The most tempting option has been to just log an error terminate any thread that attempts to misuse the class. Unfortunately, I can't find an OS-independent way of terminating just the current thread. If I was to go this route, I'd have to look up OS-appropriate ways to terminate the current threads.

I'm also not certain that I'm not being overly paranoid. Maybe I should just document the proper use of the class and if someone misuses it let them and hope that they're able to figure out why their application isn't doing what it's supposed to. I'm also worried about myself being the fool who misuses the class some day in the future.

migrated from stackoverflow.com Jan 6 '16 at 22:30

This question came from our site for professional and enthusiast programmers.

  • 1
    "not that I can understand why you'd want to perform multi-threading with an OpenGL context, as the calls needed to do so are expensive enough to negate any benefit you might get out of it from what I understand" There are many reasons for doing so. Constructing objects in the background. Loading textures in the background. Etc. This is usually done by making multiple, shared contexts. That way, each thread has its own graphics context. – Nicol Bolas Jan 6 '16 at 23:19
  • 2
    A throwing destructor is a blight upon the land and not safely usable at all in any but the most exceptional situations. Just abstain from going there. – Deduplicator Jan 6 '16 at 23:25
  • You'll get no disagreement from me. The reasons for avoiding throwing destructors are sound and valid. Thus the reason for the question, I'm trying to figure out the best way to refactor the code to remove the throwing destructors. – Darinth Jan 7 '16 at 16:28
3

So I understand that you want to be able to check for these potential problems with the thread not owning the context, but how will throwing an exception help? How do you plan to recover from the problem? And at what point in the program? My guess is that in general you can't fix the problem by the time this has happened, its just a major structural error in the program. So why not just make the best error report you can and then call std::abort, instead of throwing an exception?

Throwing exceptions from destructors violates the fundamental idea of C++ error handling, which is that destructors are used to clean up objects when an exception is thrown and the stack is being unwound. If one of those destructors throws an exception during stack unwinding for a different exception, so that there are two unresolved exceptions at the same scope, your program is terminated. In C++11, to make it safer, your program usually terminates whenever a destructor throws an exception, unless you take special boiler-plate steps to allow it. There are lots of other big problems with destructors that throw -- I have yet to see a situation where it's a good idea, or expedient in any way to make a throwing destructor.

  • The more that I'll looking at this, the more that just an error report and an abort seems like the best way of handling it. Make my basic sanity check and if something is wrong, logwhat information I can about what was done wrong and abort the application. I was thinking of terminating the thread in hopes that if this kind of an error somehow made it into a final product, it would give the application time to realize something has gone catastrophically wrong and perform some final moment operations, like attempting to save the player data to a new file. This is seeming less useful now. – Darinth Jan 7 '16 at 16:19
9

Your fundamental problem is that you've designed a contradiction.

On the one hand, your objects are RAII-encapsulated, managing resources from another system (OpenGL). But on the other hand, all of your objects silently and invisibly depend on something that isn't encapsulated: the current context. Your objects need it, but they cannot control it.

So you really only have two options:

  1. Accept reality. Your interface is fragile, and users have to use it correctly in order to get reasonable results. They must have the proper context bound when they use your objects in any way, whether creating them, using them, or destroying them. If they violate this... things blow up.

    Personally, I'd go with this option. If you're writing a graphics application that needs to talk to OpenGL, then performance is probably not unimportant to you. And this is probably the highest performing way to do it. You get reasonable RAII safety from sane use of the API.

  2. Bring the context into the abstraction. Do not give users the freedom to break your objects. One way to do this would be to make it so that objects are really owned by the context object. Thus, destruction of an object is just a signal to the context object to get rid of it later. This also means each RAII-wrapped OpenGL object becomes non-functional once the context is destroyed.

    This will generally require a lot of overhead. The RAII-wrapped objects have to talk to the context object a lot. And the context object needs to know about every RAII-wrapped object in existence. That sounds like a lot of pointer indirection, weak-references, and other non-trivially expensive things.

  • It's possible to use multiple GL contexts in a single application. In those cases where that's desired, option 2 is the only reasonable choice. – greyfade Jan 7 '16 at 18:14
  • 1
    @greyfade: Nonsense. If you're talking about multiple contexts that aren't shared, then its still up to the writer of the application to know which objects belong to which context. No safety is ever perfect; the question is merely what is "reasonable". It all depends on how you structure your code. You can build your application such that the objects that are owned by one context are hidden from the objects owned by another. Such that it's functionally impossible for them to be deleted while the wrong context is current. – Nicol Bolas Jan 7 '16 at 18:19
1

I get the impression that your goal in having the destructor throw an exception was to make it as clear as possible to library users that their program was incorrect in the way it was managing the lifetime of the object. This is a noble goal, but I have some problems with the particular approach you are using for this particular problem.

  • An exception rises up when a program encounters a run-time error. But the error was made at program-writing time (library users mismanaged your object's lifetime). So the earliest, most obvious time you can make library users aware of their error is at compile time.
  • The root of the issue is a concurrency issue. At run-time, the way the OS schedules your program's execution is non-deterministic (as far as you know). So in fact, your indication to library-users that their code uses your library incorrectly might not even appear, or it could only appear some of the time. And library users won't be able to reliably reproduce the issue (probably). So its actually a real troublesome and unreliable indicator that they did something wrong!

So my thinking is that your approach with throwing an exception in the destructor isn't really great at what you want to do anyway. I don't think you should fight the current and try to keep doing it the way you're doing it right now, or a similar way by logging things and terminating threads at run-time. I think you should move away from trying to "handle" this issue at run-time (by handle I mean "make it obvious to the programmer they need to fix their program").

You might want to re-think your architecture a little bit. I also have two more specific recommendations:

  • Make the destructor block the current thread until it can acquire the graphics engine to release the resource. Simple, but possibly undesirable if you don't want to wait to acquire the graphics engine.

  • Do graphics-engine tasks on a dedicated thread, and use a message queue pattern so other threads can dispatch certain tasks to the graphics engine.

As you can see from my recommendations, I think making your graphics engine thread-safe will save you a lot of headaches in the future even though you may have some doubts about doing that work at the moment. Both these recommendations shouldn't require a radical departure from your current architecture either (hopefully).

Hope this helps.

  • 1
    "The root of the issue is a concurrency issue." No, it isn't. Concurrency is usually not in play here. The current context is a thread-local construct, so it's not really possible to race on it. Either the context is current in the thread where the object is being destroyed or it isn't. The most you could race on would be some other thread destroying it. – Nicol Bolas Jan 6 '16 at 23:22
  • @NicolBolas The OpenGL context (wrapped by the graphics engine) is a resource which is being shared by several threads. You can race on access to the context. What do you think you're saying when you say "ether the context is current in the thread where the object is being destroyed or it isn't". What do you think determines that? It's a race condition. – dog jones Jan 7 '16 at 0:31
  • @dogjones It isn't a concurrency issue, though it might look like it superficially. It's an issue with code being accessed entirely in the wrong thread. The graphics engine is really designed to be used by only one thread. I could make it thread-safe, but the overhead of doing so I think would be unreasonable for my application. I was much more interested in making sure that something didn't get accidentally created or deleted using a thread that shouldn't be dealing with the graphics engine at all. – Darinth Jan 7 '16 at 16:08
1

This is the darker side of OpenGL, right here. I've thought about this same issue.

Fortunately, there is a relatively small number of objects types, and you could handle destruction through a simple resource manager. This resource manager would manage object lifetimes and forward them to the OpenGL library when a context is active. This is a sketch of what the code might look like.

#include <GL/gl.h>
#include <mutex>
#include <vector>

class GLResourceManager {
private:
  // Types of resources
  enum class Type { Texture, Array, Buffer };
  // Record of a resource to be freed
  struct Resource {
    Type type;
    GLuint obj;
  };
  // Mutex for this object
  std::mutex m_mutex;
  // List of resources to be freed
  std::vector<Resource> m_objs;
  typedef std::lock_guard<std::mutex> lock_guard;

public:
  // Schedule a texture object for deletion
  void DeleteTexture(GLuint tex) {
    lock_guard lock(m_mutex);
    m_objs.push_back(Resource{Type::Texture, tex});
  }
  // Schedule a vertex array object for deletion
  void DeleteArray(GLuint array) {
    lock_guard lock(m_mutex);
    m_objs.push_back(Resource{Type::Array, array});
  }
  // Schedule a buffer object for deletion
  void DeleteBuffer(GLuint buffer) {
    lock_guard lock(m_mutex);
    m_objs.push_back(Resource{Type::Buffer, buffer});
  }
  // Process all pending deletions, requires an active context
  void Run() {
    lock_guard lock(m_mutex);
    for (const Resource &r : m_objs) {
      switch (r.type) {
      case Type::Texture:
        glDeleteTextures(1, &r.obj);
        break;
      case Type::Array:
        glDeleteVertexArrays(1, &r.obj);
        break;
      case Type::Buffer:
        glDeleteBuffers(1, &r.obj);
        break;
      }
    }
    m_objs.clear();
  }
};

In the above class, you would just call DeleteTexture() in your destructor, and then call Run() once per frame, or however often you like. This approach has a number of disadvantages. The above class can still throw exceptions, but only when out of memory, and it's often okay to terminate the program when out of memory.

My personal preference is to avoid this extra work and just be careful about using OpenGL calls and objects that refer to OpenGL objects, and generous use of KHR_debug. In my applications, only the rendering thread ever makes OpenGL calls, and only that thread can construct OpenGL objects. Sometimes this means doing things like queuing state changes until the rendering thread gets around to it.

The most tempting option has been to just log an error terminate any thread that attempts to misuse the class. Unfortunately, I can't find an OS-independent way of terminating just the current thread. If I was to go this route, I'd have to look up OS-appropriate ways to terminate the current threads.

This is like saying, "I don't want to shoot myself in the foot, so I strapped a stick of dynamite to my foot, and if I ever do shoot myself in the foot, the dynamite will blow my whole leg off." Leaking OpenGL objects is bad. Terminating the program is bad. Terminating an unknown thread is far worse. Will it leave the process in a known state? Chances are low. Will the process be able to recover? Probably not. Could you end up with some kind of weird deadlock or unresponsive application? Maybe.

  • 1
    " Will it leave the process in a known state? Chances are low." I think chances are lower than that. You'd be in a destructor. I cannot honestly think of a place more likely to leave things in an unknown state when killing a thread. After all, other places don't offer the possibility of killing a thread while the stack is being unwound due to an exception. – Nicol Bolas Jan 7 '16 at 4:35
  • 1
    @NicolBolas: I can think of one place which is more likely to leave things in an unknown state: signal handler. – Dietrich Epp Jan 7 '16 at 6:45
  • @DietrichEpp The goal of terminating the offending thread over the entire application was to give the application one final chance at performing last-moment recovery options, like saving application data to a new file. All of the throwing destructors were expected to be application-terminating errors. I'm becoming less inclined towards the belief that this is necessary. – Darinth Jan 7 '16 at 17:19
  • 2
    @Darinth: "Then the application hangs" is a worse alternative than losing thirty seconds of work. Because you could just save data to disk every thirty seconds on a timer, and let the application die peacefully rather than try to do some kind of zombification and run poorly-tested recovery code inside an application which is known to have bad state, maybe while the stack is unwinding. Crash recovery is a design smell. If you need better guarantees, put them in another process with its own address space. – Dietrich Epp Jan 7 '16 at 21:18
  • 1
    And remember, you could just leak the objects and send a message to the main event loop to quit. Because "oops, I leaked an object" is not really something that needs to be handled immediately. – Dietrich Epp Jan 7 '16 at 21:20
0

In the past the way I've handled this is to create a queue of the OpenGL objects to be deleted. When the destructor gets called, instead of throwing it will add a new object to the queue that knows how to destroy the resource (by calling glDeleteBuffers, glDeleteLists, etc).

Then in your library, after each wglSwapBuffers (or equivalent) is completed, you're in code that controls the context, so have it check the "to be deleted" queue. If there are any items in the list, have them execute their cleanup and then clear the list. You will, of course, need a mutex or something to synchronize the accesses to that list.

It's a little bit messy, but sometimes that is the reality when working with external resources. With this strategy you won't need to change your API, and objects can be freed on any thread. I've used this to implement C# finalizers for objects wrapping OpenGL resources to prevent this exact threading issue.

0

I think this is strictly an OGL question, and the solution doesn't have to be so hard. I've dealt with this issue many times before, encountering the same issues years before when I first tried to wrap OGL resources into object-oriented designs -- and got burnt both ways, not just trying to destroy resources outside of a valid GL context, but also trying to create them outside of a valid context.

There's an easy solution for that. It's not as complex as it seems when you first run into this issue. Just centralize one kind of GL resource manager which actually does all the resource allocation/deallocation for the raw, low-level GL resources (VBOs, shaders, textures, etc).

Your objects then turn into like handles. Simplified example:

class GpuTexture
{
public:
    explicit GpuTexture(GpuManager& imanager): 
        manager(&imanager), texture_id(new GLuint(invalid_texture))
    {
        // The manager may not create the texture immediately
        // if it's outside a valid context. It might push it to a
        // queue of things to initialize the next time it's in a
        // valid context.
        manager->initialize_texture(texture_id);
    }

    ~GpuTexture()
    {
        // The manager may not destroy the texture immediately
        // if it's outside a valid context. It might push it to a
        // queue of things to destroy the next time it's in a
        // valid context. If the texture hasn't been initialized
        // yet, this request will simply be ignored and the texture
        // will be removed from the "to initialize" queue.
        manager->destroy_texture(texture_id);
    }

    ...

private:
    GpuManager* manager;
    shared_ptr<GLuint> texture_id;
};

That's all there is to it. You can generalize it and simplify the resource manager just passing like std::function telling what to do when a context is valid, e.g. -- make it as fancy as you like. I know people tend to discourage "Manager" names, but can't think of a more appropriate one in this case for a central GPU resource "manager".

I didn't actually use shared_ptr in the final production version (we didn't have shared_ptr yet as part of the standard back then), but that's the easiest way when combined with a safe concurrent data structure like a concurrent queue or Intel's TBB concurrent_vector. More efficient might use a fixed allocator and possibly make manager globally-accessible as kind of a singleton. I haven't found a need for that level of efficiency since most of the OGL objects I've dealt with a rather bulky (whole texture, VBO with at least thousands of triangles worth of vertex attribute data, etc).

For VBOs, you can take this concept even further and allow initialization (as in full content generation with what would normally be glBufferData and not merely VBO acqusition through glGenBuffers) of the VBO outside a valid GL context. Simply write the buffer to system memory (memcpy to a private buffer, e.g.), and then send a request to initialize that buffer to a GPU-allocated buffer through the manager when the display context is valid.

All you have to do to make life much simpler and turn what seems to be this terrible nightmare scenario into something that'll never become a thorn in your side again is to simply avoid the mentality that your object has to be the one that actually does the GPU resource allocation/deallocation. Just centralize it to one place and it'll become easy to allocate and deallocate the resources only when the context is valid, and you'll no longer have to deal with destructors that can fail because they're not in a valid context (though there's a little more work to do if we want to make it impossible to fail since pushing to the queue might fail due to OOM, e.g.).

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.