Calling virtual function from destructor - any workaround?

Question

I am porting an existing library from Windows to Linux. A few classes contain OS-specific code. I decided to not use Pimpl (*), so I went for a simple DeviceBase, DeviceWin32 and DeviceLinux hierarchy. DeviceBase contains, well, the base stuff: high-level logic, public functions and (protected) virtual doX() and doY functions (as described by Herb Sutter in his Virtuality article).

Original code (the x() below stands for all functions that have platform-specific behaviour, e.g. close(), open(), etc.):

class Device
{
public:
    Device() : _foo(42) {}
    ~Device() {
        if (!isValid()) 
            close();
    }

    bool isValid() const { return _handle != NULL && _handle != INVALID_HANDLE; }

    void x() {
        checkPrecondsForX();
        DWORD ret = DoSomethingForWindows(&_handle);
        [...]
        checkPostcondsForX();
    }

private:
    int _foo;
    HANDLE _handle;
};

New code:

class DeviceBase
{
public:
    DeviceBase() : _foo(42) {}
    virtual ~DeviceBase() {
        if (!isValid()) close();
    }

    virtual bool isValid() const = 0;

    void x() {
        checkPrecondsForX();
        doX();
        checkPostcondsForX();
    }

protected:
    virtual void doX() = 0;

private:
    int _foo;
};

class DeviceWin32
{
public:
    DeviceWin32() : DeviceBase(), _handle(NULL) {}
    virtual ~DeviceWin32() {}

    virtual bool isValid() const { return _handle != NULL && _handle != INVALID_HANDLE; }

protected:
    void doX() {
        DWORD ret = DoSomethingForWindows(&_handle);
        [...]
    }

private:
    HANDLE _handle;
};

class DeviceLinux
{
public:
    DeviceLinux() : DeviceBase(), _fd(0) {}
    virtual ~DeviceLinux() {}

    virtual bool isValid() const { return _fd != 0; }

protected:
    void doX() {
        int ret = _do_smth_posix(&_fd);
        [...]
    }

private:
    int _fd;
};

It all went smoothly until I met with the destructor. I naively assumed that, like other functions, it would work out of the box. The tests proved me wrong quite quickly, and actually crashed with a pure virtual function call. It did not take me a long time to figure out why: this is perfectly expected, documented and discussed here and elsewhere (for more examples see the column of related questions on this very page).

I am worried because, conceptually, the logic of "closing the device if it was opened" belongs to the base class. The implementation detail is the way it is represented (HANDLE vs int) and the functions to actually close it, therefore the I'd like to see the logic in the base class.

However, I could not find a workable solution in my situation. Things like "don't do this" do help understanding what is going wrong, and most of the workarounds I found boil down to either duplicating the logic of the base destructor or using a helper object (but the latter wouldn't work as we need access to data stored in derived classes).

Interestingly enough, I did not have the same problem for construction, as the call to open() does not happen from within the constructor (and this converts to using two step construction as explained in the C++ FAQ lite). This is desired because we can have an un-opened device and pass it around before it is opened at some point. The device must however be closed after use, even when exceptions occur (so RAII is the only reasonable solution).

(It may be worth noting that the DeviceXXX classes are not final (although they are usable as-is) and do have a derived class - AsyncDevice. I do not know if this could have an impact on a potential solution.)

Yes, by the time it took to write this question, I could have:

• gone for the Pimpl idiom

• written two completely separate implementations (as seen in some libs, e.g. William Woodall's Serial library)

• duplicated the isValid() check in both destructors and call it a day

• stated that RAII id bad anyway and asked the user to close() explicitly the devices (after all, they opened it, so they should be in charge of closing it).

In the quite similar question here on SO the author asks "Is this true ?". Now I am asking: "Does anyone knows of a solution that does not imply a complete redesign of the library (e.g. Pimpl) and/or logic duplication" ? I am sorry if this sounds like nitpicking but I wouldn't want to miss something (almost) obvious.

(*) This might sound stupid - if I had, I would obviously not be facing this specific problem. Maybe some others, though. But anyway, I just don't want to squander an opportunity to learn something.

UPDATE: The 3 answers so far push towards having two separate classes to manage the different concerns (something I had not identified clearly - thanks for pointing this out). I am trying this approach although I have the feeling it will be slightly harder to manage the 2nd inheritance (AsyncDevice, which may now need more work). We'll see where I end up.

UPDATE 2: I finally took a step back and made what I think is a wise decision given the situation: accept code duplication, and keep the answers calling for a separation of concerns under my pillow for future use. Therefore the two destructors of DeviceWin32 and DeviceLinux are very similar - but two instances "only" are not enough to justify a refactor/generalization. Many thanks to those who answered.

Answer 1

This may not be the answer you want, but I feel it's important to post it anyway.

There are (at least) two separate concerns in your problem domain. One is the lifetime of the device and the other is its internal state.

As you will know, it is considered good practice in c++ to give each class exactly one concern, or 'job'.

Thus unhappily for you, the correct solution is to separate concerns, in this case into a non-virtual device_handle class which owns a virtual device_concept, which can then be derived from.

so:

  // concern 1 : internal state
  struct device_concept {
    virtual ~device_concept();
    virtual bool is_open() const = 0;
    virtual void close() = 0;
    virtual void doX() = 0;
  };

struct windows_device : public device_concept {... };
struct linux_device : public device_concept {... };

// concern 2 : lifetime
struct device_handle
{
#if WINDOWS
  device_handle() 
  : _ptr(new windows_device, &close_and_delete)
  {}
#else ... etc
#endif    

  // non virtual functions deferring to virtual ones
  void doX()
  {
    _ptr->doX();
  }    

  private:
    static void close_and_delete(device_concept* p) {
      if (p && p->is_open()) {
        p->close();
      }
      delete p;
    }
    std::unique_ptr<device_concept, void(*)(device_concept*)> _ptr;
};

Answer 2

Can you separate control of the scope lifetime of the object from the object itself, allowing you to use RAII ?

I've tried to follow your example where base class is effectively the logic (x), derived classes are the implementation (x and open/close) and the guard class manages lifetime of open/close calls.

#include <iostream>
#define WIN32_LEAN_AND_MEAN
#include <Windows.h>
namespace ExampleOpenCloseScope
{
    static DWORD DoSomethingForWindows(HANDLE *noonePassesHandlesLikeThis) { *noonePassesHandlesLikeThis = reinterpret_cast<HANDLE>(1); std::wcout << L"DoSomethingForWindows()\n";  return 0; }
    static void WindowsCloseHandle(HANDLE /*handle*/) { std::wcout << L"WindowsCloseHandle()\n"; }
    class DeviceBase
    {
        int _x;
    public:
        bool isValid() const { return isValidImpl(); }
        void x() { checkPrecondsForX(); doX(); checkPostcondsForX(); }
        bool open() { return openHandle(); }
        void close() { closeHandle(); }
    private:
        virtual bool isValidImpl() const = 0;
        virtual void checkPrecondsForX() = 0;
        virtual void doX() = 0;
        virtual void checkPostcondsForX() = 0;

        virtual bool openHandle() = 0;
        virtual void closeHandle() = 0;
    protected:
        DeviceBase() : _x(42) {}
    public:
        virtual ~DeviceBase() = 0 {}
    };

    class DeviceWin32 : public DeviceBase  
    {
    private:
        HANDLE _handle;
        virtual bool isValidImpl() const override { return _handle != NULL; }
        virtual void checkPrecondsForX() override { std::wcout << L"DeviceWin32::checkPrecondsForX()\n"; }
        virtual void doX() override { std::wcout << L"DeviceWin32::doX()\n"; }
        virtual void checkPostcondsForX() override { std::wcout << L"DeviceWin32::checkPostcondsForX()\n"; }
        virtual bool openHandle() override { std::wcout << L"DeviceWin32::openHandle()\n"; if (_handle == NULL) return DoSomethingForWindows(&_handle) == ERROR_SUCCESS; return true; }
        virtual void closeHandle() override { std::wcout << L"DeviceWin32::closeHandle()\n"; if (_handle != NULL) WindowsCloseHandle(_handle); _handle = NULL; }
    public:
        DeviceWin32() : _handle(NULL) {}
        virtual ~DeviceWin32() { std::wcout << L"DeviceWin32::~DeviceWin32()\n"; }
    };

    static int _do_smth_posix(int *fd) { *fd = 1; std::wcout << L"_do_smth_posix()\n"; return 0; }
    static void _posix_close_fd(int /*fd*/) { std::wcout << L"_posix_close_fd\n"; }

    class DeviceLinux : public DeviceBase
    {
    private:
        int _fd;
        virtual bool isValidImpl() const override { return _fd != 0; }
        virtual void checkPrecondsForX() override { std::wcout << L"DeviceLinux::checkPrecondsForX()\n"; }
        virtual void doX() override { std::wcout << L"DeviceLinux::doX()\n"; }
        virtual void checkPostcondsForX() override { std::wcout << L"DeviceLinux::checkPostcondsForX()\n"; }
        virtual bool openHandle() override { std::wcout << L"DeviceLinux::openHandle()\n"; if (_fd == -1) return _do_smth_posix(&_fd) == 0; return true; }
        virtual void closeHandle() override { std::wcout << L"DeviceLinux::closeHandle()\n"; if (_fd != -1) _posix_close_fd(_fd); _fd = -1; }
    public:
        DeviceLinux() : _fd(-1) {}
        virtual ~DeviceLinux() { std::wcout << L"DeviceLinux::~DeviceLinux()\n"; }
    };

    class DeviceGuard
    {
        DeviceBase *_device;
        bool _open;
    public:
        DeviceGuard(DeviceBase *device) : _device(device) { _open = _device->open(); }
        ~DeviceGuard() { try { if (_open) _device->close(); _open = false; } catch (...) { std::wcerr << L"This ain't good\n"; } }

        DeviceGuard(DeviceGuard const &) = delete;
        DeviceGuard & operator=(DeviceGuard const &) = delete;
    };

    enum OS
    {
        Windows,
        Linux
    };
    static OS GetOs() { return OS::Windows; }
    void TestDevice(DeviceBase *device)
    {
        DeviceGuard guard(device);
        device->x();
    }
    void Test()
    {
        std::wcout << L"===ExampleOpenCloseScope.Test()===\n";

        DeviceBase *device;
        if (GetOs() == Windows) device = new DeviceWin32();
        else device = new DeviceLinux();

        TestDevice(device);
        delete device;
        std::wcout << L"exiting ExampleOpenCloseScope.Test()\n";
    }
}

The output is:

===ExampleOpenCloseScope.Test()===
DeviceWin32::openHandle()
DoSomethingForWindows()
DeviceWin32::checkPrecondsForX()
DeviceWin32::doX()
DeviceWin32::checkPostcondsForX()
DeviceWin32::closeHandle()
WindowsCloseHandle()
DeviceWin32::~DeviceWin32()
exiting ExampleOpenCloseScope.Test()

Answer 3

How about you wrap your device handle in its own class that exposes the following methods?

public class ADeviceHandle {
    virtual bool IsValid() = 0;
    void* GetHandle() = 0;
}

The implementation details of these are handle specific, your current classes need only call the above two methods and keep a member of ADeviceHandle?