Does it make sense to check for nullptr in custom deleter of shared_ptr?

Question

I've seen some code that uses std::shared_ptr with a custom deleter that test the argument for nullptr, for example, MyClass which has a close() method and is constructed with some CreateMyClass:

auto pMyClass = std::shared_ptr<MyClass>(CreateMyClass(), 
                                        [](MyClass* ptr)
                                        { 
                                            if(ptr) 
                                                ptr->close(); 
                                        });

Does it make sense to test ptr for null-ness in the deleter? Can this happen? how?

Answer 1

The constructor std::shared_ptr<T>::shared_ptr(Y*p) has the requirement that delete p is a valid operation. This is a valid operation when p equals nullptr.

The constructor std::shared_ptr<T>::shared_ptr(Y*p, Del del) has the requirement that del(p) is a valid operation.

If your custom deleter cannot handle p being equal to nullptr then it is not valid to pass a null p in the constructor of shared_ptr.

The constructor you offer as an example can be better presented, thus:

#include <memory>

struct MyClass {
    void open() {
        // note - may throw
    };

    void close() noexcept {
        // pre - is open
    }
};

struct Closer
{
    void operator()(MyClass* p) const noexcept
    {
        p->close();
        delete p;  // or return to pool, etc
    }
};

auto CreateMyClass() -> std::unique_ptr<MyClass, Closer>
{
    // first construct with normal deleter
    auto p1 = std::make_unique<MyClass>();

    // in case this throws an exception.
    p1->open();

    // now it's open, we need a more comprehensive deleter
    auto p = std::unique_ptr<MyClass, Closer> { p1.release(), Closer() };
    return p;
}

int main()
{
    auto sp = std::shared_ptr<MyClass>(CreateMyClass());
}

Note that it is now not possible for the shared_ptr to own a null object.

Answer 2

Yes, it makes sense actually. Suppose CreateMyClass returns nullptr. Reference count of pMyClass (use_count) becomes 1. When pMyClass will be destroyed, following will happens:

If *this owns an object and it is the last shared_ptr owning it, the object is destroyed through the owned deleter.

So if custom deleter dereferencing a pointer that holded by shared_ptr (ptr->close() in your code) then it should take care of nullptr checking.

Notice that empty shared_ptr is not the same as null shared_ptr.

Answer 3

struct deleter {
  template<class T>
  void operator()(T*) const {
    std::cout << "deleter run\n";
  }
};

int main() {
  std::shared_ptr<int> bob((int*)0, deleter{});
}

Live example.

This prints "deleter run\n". The deleter is indeed run.

The concept of empty and the concept of owning a nullptr are distinct concepts for shared_ptr.

bob is non-empty, yet bob.get()==nullptr. When non-empty, the destructor is called.

int main() {
  int x;
  std::shared_ptr<int> alice( std::shared_ptr<int>{}, &x );
}

alice is empty, yet alice.get() != nullptr. When alice goes out of scope, delete &x is not run (and in fact no destructor is run).

This can only be avoided if you never construct your shared pointer with a null pointer and a deleter.

One way to approach this is to first create a unique pointer with a custom deleter.

template<class Deleter, class T>
std::unique_ptr<T, Deleter> change_deleter( std::unique_ptr<T> up, Deleter&& deleter={} ) {
  return {up.release(), std::forward<Deleter>(deleter)};
}

struct close_and_delete_foo; // closes and deletes a foo

std::unique_ptr<foo, close_and_delete_foo> make_foo() {
  auto foo = std::make_unique<foo>();
  if (!foo->open()) return {};
  return change_deleter<close_and_delete_foo>(std::move(foo));
}

Unlike shared_ptr, unique_ptr cannot hold nullptr yet be "non-empty" (the standard doesn't use the term empty for unique_ptr, instead it talks about .get()==nullptr).

unique_ptr can be implicitly converted to a shared_ptr. If it has nullptr, the resulting shared_ptr is empty, not just holding nullptr. The destroyer of the unique_ptr is carried over to the shared_ptr.

---

The downside to all of these techniques is that the shared_ptr reference counting memory block is a separate allocation to the object's memory block. Two allocations is worse than one.

But the make_shared constructor doesn't let you pass in a custom deleter.

If destroying your object cannot throw, you can use the aliasing constructor to be extremely careful:

// empty base optimization enabled:
template<class T, class D>
struct special_destroyed:D {
  std::optional<T> t;
  template<class...Ds>
  special_destroyed(
    Ds&&...ds
  ):
    D(std::forward<Ds>(ds)...)
  {}
  ~special_destroyed() {
     if (t)
       (*this)(std::addressof(*t));
  }
};
std::shared_ptr<MyClass> make_myclass() {
  auto r = std::make_shared< special_destroyed<MyClass, CloseMyClass> >();
  r->t.emplace();
  try {
    if (!r->t->open())
      return {};
  } catch(...) {
    r->t = std::nullopt;
    throw;
  }
  return {r, std::addressof(*r.t)};
}

Here we manage to use one block for destroyer and reference counting, while permitting a possibly failing open operation, and automatically closing only when the data is actually there.

Note that the destroyer should only close the MyClass, not delete it; deleting is handled by an outer destroyer in the make_shared wrapping the special_destroyed.

This uses C++17 for std::optional, but alternative optionals are available from boost and elsewhere.

---

A raw C++14 solution. We create a crude optional:

template<class T, class D>
struct special_delete:D {
  using storage = typename std::aligned_storage<sizeof(T), alignof(T)>::type;
  storage data;
  bool b_created = false;
  template<class...Ts>
  void emplace(Ts&&...ts) {
    ::new( (void*)&data ) T(std::forward<Ts>(ts)...);
    b_created=true;
  }
  template<std::size_t...Is, class Tuple>
  void emplace_from_tuple( std::index_sequence<Is...>, Tuple&&tup ) {
    return emplace( std::get<Is>(std::forward<Tuple>(tup))... );
  }
  T* get() {
    if (b_created)
      return reinterpret_cast<T*>(&data);
    else
      return nullptr;
  }
  template<class...Ds>
  special_delete(Ds&&...ds):D(std::forward<Ds>(ds)...){}
  ~special_delete() {
    if (b_created)
    {
      (*this)( get() );
      get()->~T();
    }
  }
};
struct do_nothing {
  template<class...Ts>
  void operator()(Ts&&...)const{}
};

template<class T, class D, class F=do_nothing, class Tuple=std::tuple<>, class...Ds>
std::shared_ptr<T> make_special_delete(
  F&& f={},
  Tuple&& args=std::tuple<>(),
  Ds&&...ds
) {
  auto r = std::make_shared<special_delete<T,D>>(std::forward<Ds>(ds)...);
  r->emplace_from_tuple(
    std::make_index_sequence<
      std::tuple_size<std::remove_reference_t<Tuple>>::value
    >{},
    std::move(args)
  );
  try {
    f(*r->get());
  } catch(...) {
    r->b_created = false;
    r->get()->~T();
    throw;
  }
  return {r, r->get()};
}

This is probably going too far. Luckily our extremely limited optional can be written easier than a real optional, but I'm not certain I did it all right.

Live example.

The C++11 version requires manually writing make_index_sequence etc.