When using "new" to initialize unique_ptr is the custom deleter responsible for freeing that memory?

Question

Smart Pointers are a new concept for me. I have been trying to wrap a File class around fopen_s and fclose using a smart pointer with a custom deleter (unique_ptr).

Below is my attempt. It successfully compiles, runs, and generates a file named "text.txt" with the contents "Hello World" as expected.

I had to use "new" to initialize unique_ptr in my Open function since make_unique does not appear work with custom deleters. Since I am using "new" is my custom deleter responsible for freeing that allocated memory?

I have stepped through my program (VS2019). File::Close only gets called once. I expected it to be called when "handle" in my File:Open function went out of scope, but this was not the case. This behavior may be influenced by the call to std::move(). Not sure how to investigate further on what happens here.

#include <Windows.h>
#include <memory>
#include <string>
#include <map>

class File
{

private:

//functors - custom deleter
  struct Close { void operator()(FILE** _handle); };

//type definitions
  typedef std::unique_ptr<FILE*,File::Close> Handle;
  typedef std::map<std::string,Handle> HandleMap;

//static members
  static Handle& Open(std::string _name, std::string _mode);
  static HandleMap s_handle_map_;

//variables
  Handle& handle_;
  std::string name_;

public:

//functions
  File(std::string _name, std::string _mode);
  void Write(std::string _message);

};

File::HandleMap File::s_handle_map_;

File::File(std::string _name, std::string _mode)
:handle_(Open(_name,_mode)),
 name_(_name)
{
}

File::Handle& File::Open(std::string _name, std::string _mode)
{
  bool exist = s_handle_map_.count(_name) > 0;

  if (!exist)
  {
    Handle handle(new FILE*(nullptr));

    //open new file
    fopen_s(
      handle.get(),
      _name.c_str(),
      _mode.c_str()
    );

    //transfer ownership of handle
    s_handle_map_.emplace(
      _name,
      std::move(handle)
    );

  }

  return s_handle_map_[_name];
}

void File::Close::operator()(FILE** _handle)
{
  fclose(*_handle);
  *_handle = nullptr;

  //necessary?
  delete _handle;
  _handle = nullptr;
}

void File::Write(std::string _message)
{
  fprintf(*handle_, _message.c_str());
}

int WINAPI WinMain(HINSTANCE _instance, HINSTANCE _previous, LPSTR _cmd, int _show)
{
  File file("test.txt","w");
  file.Write("Hello World\n");
  return 0;
}

whoa that's complicated. And `s_handle_map_` appears to have no purpose except to create dangling handles. — Mooing Duck, Oct 25 '19 at 21:08
Why do you want to allocate something with `new`? File handles are opened and closed rather than allocated and deallocated. — Yksisarvinen, Oct 25 '19 at 21:08
Since you use `delete` manually and explicitly, you failed to delegate resource cleanup to a smart pointer. — Ulrich Eckhardt, Oct 25 '19 at 21:08
Windows has a `HANDLE` type, but what's `Handle`? `std::move(handle)` probably doesn't do what you think it does — Mooing Duck, Oct 25 '19 at 21:10
@Mooing Duck The static map's purpose is to allow creating multiple File objects that point to the same opened file. — Skrug, Oct 25 '19 at 21:17
You probably want to rethink your approach. If you are using a custom deleter, you probably should also be using a custom allocator. If you are using `new` as an allocator, you probably want the standard deleter. In your case, I'd think the latter, as you are trying a complex version of [RAII](https://stackoverflow.com/questions/2321511/what-is-meant-by-resource-acquisition-is-initialization-raii). — JaMiT, Oct 25 '19 at 21:19
@Yksisarvinen How do I initialize std::unique_ptr using fopen_s then? I think the fopen_s function is what makes this complicated, since it does not appear that I can simply use std::unique_ptr. Let me know if I am overlooking something. — Skrug, Oct 25 '19 at 21:20
@Mooing Duck Yes. Windows has a HANDLE. File::Handle is just a typedef of std::unique_ptr. This line as included in the example above. I am under the impression that std::move changes ownership of the unique_ptr. Is this not the case? — Skrug, Oct 25 '19 at 21:28
Possible duplicate of [Is unique\_ptr guaranteed to store nullptr after move?](https://stackoverflow.com/questions/24061767/is-unique-ptr-guaranteed-to-store-nullptr-after-move) — JaMiT, Oct 25 '19 at 21:31
@Skrug Open the `FILE` normally: `FILE *f = nullptr; fopen_s(&f, ...);` Then you can construct the `unique_ptr` with a custom deleter like this: `std::unique_ptr fptr(f, &::fclose);` Or this: `auto del = [](FILE *f){ ::fclose(f); }; std::unique_ptr fptr(f, del);` — Remy Lebeau, Oct 25 '19 at 21:39
@UlrichEckhardt Yes, this seems to go against the purpose of using a smart pointer. But is the deleter, in this case, responsible for freeing this memory? If not, then I should remove the call to delete and the purpose of using a smart pointer is restored. What makes this confusing I think is that this is not a unique pointer to a FILE (unique_ptr) but instead it is a unique pointer to a FILE pointer (unique_ptr). fopen_s requires a FILE** so unique_ptr does not seem to be an option. — Skrug, Oct 25 '19 at 21:40
@Skrug "*this is not a unique pointer to a `FILE` (`unique_ptr`) but instead it is a unique pointer to a `FILE` pointer (`unique_ptr`)*" - that is where you are making your mistake."*`fopen_s` requires a `FILE**` so `unique_ptr` does not seem to be an option*" - `fopen_s()` takes a `FILE**` because it outputs a `FILE*`, the same thing that `fopen()` returns, so it needs an extra level of indirection to write out that value. Once the file is opened, all subsequent use of the `FILE` is done using a `FILE*` not a `FILE**`. So `unique_ptr` will work just fine for that. — Remy Lebeau, Oct 25 '19 at 21:50
@RemyLebeau this is essentially what I am doing but FILE* f goes out of scope and mangles fptr (exceptions occur on call to fprintf. so f needs to be changed to FILE** f = new FILE*(nullptr) which I am already doing. — Skrug, Oct 25 '19 at 22:07
"*`FILE* f` goes out of scope and mangles `fptr`*" - no, it doesn't. `f` is just a raw pointer, the `unique_ptr` will make a copy of it for its own use. "*exceptions occur on call to `fprintf`*" - then you are misusing it. "*`f` needs to be changed to `FILE** f = new FILE*(nullptr)`*" - no, it doesn't. You clearly don't understand how pointers work. I agree with MooringDuck, you have made the code way more complicated than it needs to be, and you are mismanaging things. — Remy Lebeau, Oct 25 '19 at 22:32
@rustyx `fclose()` is the correct way to close and deallocate a `FILE` struct — Remy Lebeau, Oct 25 '19 at 22:52

Christophe · Answer 1 · 2019-10-25T22:27:02.277

Whenever you think of unique_ptr<FILE*, ...>, take a deep breath in, wait a minute, then go on with fstream.

The following code does the same thing, but relies on a proven and well tested C++ standard library. fstream have all the features you expect, including automated closing when they are no longer needed:

int WINAPI WinMain(HINSTANCE _instance, HINSTANCE _previous, LPSTR _cmd, int _show)
{
  fstream file("test.txt", fstream::out);
  file << "Hello World\n";
  return 0;
}

And you do not need to worry about memory management at all.

Now, generalizing your question :

if you create a unique_ptr<T,D> yourself based on a new T pointer, the custom deleter D will have the responsibility to delete T. If you don't, you will leak memory (example).
A better approach would therefore be to keep using the default deleter, and make sure that T's destructor will clean or close anything needed.
And once you go for a default deleter, the best would be to go for make_unique that has some advantages over the new approach (see here why)

@RemyLebeau that was indeed my first thought as well. But I preferred the more general `fstream` since the OP's `File` class allows to chose the mode. — Christophe, Oct 25 '19 at 21:54

score 1 · Accepted Answer · answered Oct 25 '19 at 22:51

You are making your use of std::unique_ptr more complicated than it needs to be. DO NOT store a FILE** pointer inside the unique_ptr, store a FILE* instead. That is what fopen_s() outputs, and all access to the FILE is done through FILE* not FILE**. You don't need 2 levels of indirection when 1 level will suffice.

Try this:

#include <Windows.h>
#include <memory>
#include <string>
#include <map>

class File
{
private:

//functors - custom deleter
  struct Close { void operator()(FILE* f); };

//type definitions
  typedef std::unique_ptr<FILE,File::Close> Handle;
  typedef std::map<std::string,Handle> HandleMap;

//static members
  static Handle& Open(std::string _name, std::string _mode);
  static HandleMap s_handle_map_;

//variables
  Handle& handle_;
  std::string name_;

public:

//functions
  File(std::string _name, std::string _mode);
  void Write(std::string _message);

};

File::HandleMap File::s_handle_map_;

File::File(std::string _name, std::string _mode)
 : handle_(Open(_name,_mode)), name_(_name)
{
}

File::Handle& File::Open(std::string _name, std::string _mode)
{
  auto iter = s_handle_map_.find(_name);

  if (iter == s_handle_map_.end())
  {
    FILE *f = nullptr;

    //open new file
    if (fopen_s(&f, _name.c_str(), _mode.c_str()) != 0)
        throw std::runtime_error("cannot open file");

    //transfer ownership of handle
    iter = s_handle_map_.emplace(_name, Handle(f)).first;
  }

  return iter->second;
}

void File::Close::operator()(FILE* f)
{
  if (f)
    fclose(f);
}

void File::Write(std::string _message)
{
  fprintf(handle_.get(), "%s", _message.c_str());
}

int WINAPI WinMain(HINSTANCE _instance, HINSTANCE _previous, LPSTR _cmd, int _show)
{
  File file("test.txt", "w");
  file.Write("Hello World\n");
  return 0;
}

Yes, this is the proper solution. Further review of unique_ptr documentation has lead me to this understanding : The unique_ptr destructor does not free memory, the deleter does. The destructor calls `get_deleter()` which uses `std::default_delete::operator()(T* ptr)` by default to free memory. So if you provide your own deleter, your replacing the default deleter and are now responsible for freeing the memory yourself. My original solution had an unnecessary layer of indirection, so `fclose()` and `delete` were necessary. This solution only requires a call to `fclose()`. — Skrug, Oct 26 '19 at 00:12

When using "new" to initialize unique_ptr is the custom deleter responsible for freeing that memory?

2 Answers2