Is a "catch all" block which deallocates dynamically allocated memory and then rethrows a valid/good design choice?

Question

I am usually making use of modern C++'s features like smart pointers and rarely using raw pointers as handlers for dynamically allocated objects. Therefore, I don't have much experience with deallocation. I've wondered whether the following code example would be a valid choice of design to prevent exception induced memory leak:

void HttpListener::spawnRequestHandler(const http_request& request) {
    std::thread handlerThread([request](){
        IRequestHandler* handler = new HttpRequestHandler(request);
        try {
            handler->handleRequest();
        }
        catch (...){
            delete handler;
            std::rethrow_exception(std::current_exception());
        }
    });
    handlerThread.detach();
}

Use `unique_ptr` rather than naked new, The `unique_ptr` will free up its pointer when it goes out of scope, even in the case of an exception. — jkb, Jun 26 '20 at 05:24
@jkb It seems to me that he is already aware of the unique_ptr technique, and specifically asking what should be done when handling the raw pointer. — K.R.Park, Jun 26 '20 at 05:54
_"I am usually making use of modern C++'s features like smart pointers"_ - Why not here? It would be a good use case for smart pointers. — Lukas-T, Jun 26 '20 at 06:06
@K.R.Park Smart pointer would reduce the lambda function to only two lines. There is no reason to use raw pointer in this situation. — stefan.gal, Jun 26 '20 at 06:14
I think his point is more to learn the backgrounds. So from a purely educational standpoint, it makes sense to me to work with raw pointers. — Ben1980, Jun 26 '20 at 06:18
No. Allocate the handler object on the stack instead of dynamically and it will automatically be destroyed on any exit from the enclosing scope. — user207421, Jun 26 '20 at 06:52
By the way, you can write `throw;` in place of the C++11 `std::rethrow_exception(std::current_exception());`. `throw;` throws the currently caught exception *by reference*. — Bathsheba, Jun 26 '20 at 07:12
It looks like a memory leak (cannot be 100% sure though) - please see my add-on answer — darune, Jun 26 '20 at 07:56
@Bathsheba That's really nice to know actually. However, I plan on storing exceptions that have been thrown inside the RequestHandler's thread in a vector of exception pointers and rethrow them later in the main thread, so I think throwing them by reference would add some questions about the exception's lifetime. — Max Luchterhand, Jun 28 '20 at 08:35

darune · Answer 1 · 2020-06-26T09:41:15.943

3

Here is already a good answer.

However, it seems there are more issues with the code. Where is the request handler raw pointer supposed to be cleaned up ?

Maybe it gets owned by http_request (unlikely) or deletes itself inside handleRequest()(also unlikely), we cannot see this from the example (but both would be bad practice). It looks like a memory leak.

Also there is no need to explicitly use the interface IRequestHandler.

To sum up, the code (in the thread) might be simplified to:

HttpRequestHandler handler(request);
handler.handleRequest();

Additionally you do not need pointers to base class, you can use references just as well:

IRequestHandler& handlerInterface = handler;

edited Jun 26 '20 at 09:41

answered Jun 26 '20 at 07:54

darune

10,480
2
24
62

Thanks a lot for your answer. I actually forgot to clean up the handler pointer. The reason I originally used a dynamically allocated object was because I was still in the mind of "an object allocated on the stack is always destroyed after the function block ends", not considering that in this case, the stack would be in another thread and not affected by closing the block of the spawnRequestHandler function. – Max Luchterhand Jun 28 '20 at 08:52

score 2 · Accepted Answer · answered Jun 26 '20 at 06:14

What you suggest will work, however, I would recommend against it. C++ is one of very few languages that has the concept of RAII, which is created to do exception safe code.

In short, of you have a call, it gets/creates the owning thing it's constructor and does the cleanup in it's destructor.

Some very good examples of this are str::unique_ptr and std::scoped_lock. However this is also very useful if you have multiple return statements.

In this case, I would adapt the code to:

void HttpListener::spawnRequestHandler(const http_request& request) {
    std::thread handlerThread([request](){
        auto handler = std::make_unique<HttpRequestHandler>(request);
        handler->handleRequest();
        handler.release();
    });
handlerThread.detach();
}

As you can see, the code is smaller and easier to read. And you can stop the delete after all ended correctly via the release. Not sure if that's a big in your original code. Though, if intended, this makes it explicit, which is easier to debug afterwards.

If you need other actions that need to happen at destruction, scope guard can be very useful. Not sure about the link, though, the presentation by Andrei Alexandrescu this is based on, had all features you can imagine.

Note: if you don't need the release, you can as well create the instance on the stack.

That solution is definitely better, however I was intentionally avoiding smart pointers because I was under the impression that I needed to use raw pointers to use dynamic polymorphism. I forgot about std::dynamic_pointer_cast, so I reckon I am going to use shared pointers — Max Luchterhand, Jun 26 '20 at 06:40
To up cast, you don't need a raw pointer. And even if you need a raw pointer, you can still store the ownership in the smart pointer and retrieve the raw pointer to pass along. Simple tip: No raw pointers with ownership. — JVApen, Jun 26 '20 at 06:43
@stefan.gal Looking at the original code, it can't if you want to keep behavior. It might be a bug — JVApen, Jun 26 '20 at 09:31

Ben1980 · Answer 3 · 2020-06-26T06:07:44.630

I would suggest to also put the memory allocation into the try block and be more specific on which exceptions to catch, e.g.

IRequestHandler* handler = nullptr;
try {
    handler = new HttpRequestHandler(request);
    handler->handleRequest();
}
catch(const std::bad_alloc& e) {
    log("Not enough heap memory."); //however you log or use cout
}
catch(const HttpRequestHandlerExeption1& e) {
    delete handler;
    HttpRequestHandlerstd::rethrow_exception(std::current_exception());
}
.
.
.
delete handler;

A good post about catching multiple exceptions you can find here

Is a "catch all" block which deallocates dynamically allocated memory and then rethrows a valid/good design choice?

3 Answers3