How to properly use the custom shared_ptr deleter?

I'm still a little confused about the proper way to use a custom deleter with shared_ptr. I have a ResourceManager class which keeps track of resource allocations, and I modified its interface to support automatic release of used resources by making the Release method private, and Allocate method returning a ResourceHolder:

// ResourceManager.cpp:
public:
    ResourceHolder<Resource> Allocate(args);

private:
    void Release(Resource*);

And the ResourceHolder class I implement like this:

// ResourceHolder.h
template <typename T>
class ResourceHolder
{
public:
    ResourceHolder(
        _In_ T* resource,
        _In_ const std::function<void(T*)>& cleanupFunction)
        : _cleanupFunction(cleanupFunction)
        , _resource(resource, [&](T* resource)
        { 
            cleanup(resource); 
        }) // Uses a custom deleter to release the resource.
    {
    }

private:
    std::function<void(T*)> _cleanupFunction;
    std::shared_ptr<T> _resource;
};

// ResourceManager::Allocate()
...
return ResourceHolder<Resource>(new Resource(),[this](Resource* r) { Release(r); });

In my cleanup method, do I have to delete T? Is it always safe to do it?
```
if (nullptr != T) delete T;
```
What happens if cleanup() can throw an exception? Can I let it escape the scope under some circumstances, or should I always prevent it?

My ResourceManager does not have a dependency on a tracing library I'm using, so I opted for a callback which a caller can provide through its constructor, and which will get called in the release method. So my Release looks something like this:

void Release(Resource* r)
{
    shared_ptr<std::Exception> exc = nullptr;
    try
    {
        // Do cleanup.
    }
    catch(Exception* ex)
    {
        exc.reset(ex);
    }

    if (nullptr != r) delete r;

    // Is it now safe to throw?
    if (nullptr != m_callback)
        m_callback(args, exc);
}

void Callback(args, shared_ptr<std::Exception> ex)
{
    // Emit telemetry, including exception information.

    // If throwing here is ok, what is the correct way to throw exception here?
    if (nullptr != ex)
    {
        throw ex;
    }
}

Is this a sound design approach?

Solution

In my cleanup method, do I have to delete T? Is it always safe to do it?

If the pointer references an object instantiated with new then you need to call delete otherwise you end up with a memory leak and undefined behavior.

What happens if cleanup() can throw an exception? Can I let it escape the scope under some circumstances, or should I always prevent it?

It shouldn't and you should make every effort to ensure that it doesn't. However if the cleanup code does throw an exception you should catch it, handle it appropriately, and eat it. The reason is the custom deleter can be invoked while in the context of a destructor and there is always the possibility the destructor is being invoked while an exception is already being propagated. If an exception is already in progress and another uncaught exception is thrown the application will terminate. In other words treat the custom deleter and cleanup code as if it were a destructor and follow the same rules and guidelines regarding exception handling.

Effective C++ Item #8 - Prevent exceptions from leaving destructors

Destructors should never emit exceptions. If functions called in a destructor may throw, the destructor should catch any exceptions, then swallow them or terminate the program.

§ 15.1/7 C++ Standard [except.throw]

If the exception handling mechanism, after completing evaluation of the expression to be thrown but before the exception is caught, calls a function that exits via an exception, std::terminate is called.

Is this a sound design approach?

With the exception of how you currently intend on handling exceptions, I see nothing wrong with it. The only real changes you need to make is how you are invoking the callback and how the callback handles the exception passed to it. The resulting code after the changes might look something like below.

void Release(Resource* r)
{
    try
    {
        // Do cleanup.
    }
    catch (Exception& ex)
    {
        // Report to callback
        if (nullptr != m_callback)
            m_callback(args, &ex);

        // Handle exception completely or terminate

        // Done
        return;
    }

    // No exceptions, call with nullptr
    if (nullptr != m_callback)
        m_callback(args, nullptr);
}

void Callback(args, const Exception* ex)
{
    // Emit telemetry, including exception information.

    //  DO NOT RETHROW ex
}