Deconstructor throwing exception

Hi, I have the following code within my main class, and this bit of code is from the deconstructor. Now this interface IConnectionPoint *m_pCP can return an error if the Unadvise method fails. Now should I throw this error from the deconstructor, or.... well... how else would I deal with this if an error did occur from the method? The return type is a HRESULT.

Code:

---

  m_hrRetResult = m_pCP->Unadvise(m_ulAdvise);
  if(FAILED(m_hrRetResult)) throw m_hrRetResult;

---

Thanks.

>Now should I throw this error from the deconstructor
Not advisable. If an exception is thrown from a destructor and not caught in the destructor, it won't run to completion. And that's a Bad Thing. A good way to avoid exceptions from propagating outside of your destructor is to wrap the body in a try block with an empty catch block that catches (...). To handle the error handling part, you can try setting a (gasp!) global variable to mark the error.

Yeah, thats probably the best way of dealing with the error. However, I dont like the idea of a global variable. Even if an error does occur from the function call, the caller cant really do anything constructive with it, even if they have the error code. So I might omit the checking.

>the caller cant really do anything constructive with it, even if they have the error code. So I might omit the checking.
If you do, still use the try block so that the exception doesn't leave the destructor. If it was called through normal operation and an exception is thrown then the destructor won't finish its job. If it was called due to a thrown exception and the stack unwinding, throwing another exception will result in terminate being called. Neither of those are good things for a robust program. ;)

Ok, so the try and catch block is a good idea to implement for all deconstructors? The trouble with the class that I am working on at the moment, is that it is all COM based. Therefore, as far as I am aware, none of the interfaces throw exceptions, they all return HRESULTS. Nevertheless, I suppose an exception could arise from within my class by using the delete operator in the deconstructor.

>Ok, so the try and catch block is a good idea to implement for all deconstructors?
Any destructor that might throw an exception.

Quote:

---

Originally Posted by Prelude

A good way to avoid exceptions from propagating outside of your destructor is to wrap the body in a try block with an empty catch block that catches (...). To handle the error handling part, you can try setting a (gasp!) global variable to mark the error.

---

Could you explain that again? I'm somewhat confused what is being wrapped in the try block, and from what part of the code the errors are coming from.

>I'm somewhat confused what is being wrapped in the try block, and from what part of the code the errors are coming from.

Code:

---

something::~something()
{
  try {
    // Stuff that might throw an exception
  }
  catch (...) {
    // Do nothing, or set a flag
  }
}

---

If this class is the event sink, you shouldn't be calling Unadvise from the destructor. Unadvise should be called from a member or static function and the object's lifetime controlled by reference counting. That is, it should not be explicitly destroyed, except from IUnknown::Release with delete this.

If you're calling Unadvise for an event sink implemented elsewhere you can ignore this post.

I dont think this is the case, because I am calling Unadvise from a ptr to my event sink class which is a member of the main class, and the destructor code was taken from this main class:

Code:

---

if(m_pCP)
{
  m_hrRetResult = m_pCP->Unadvise(m_ulAdvise);
  SAFE_RELEASE(m_pCP);
}

if(m_TapiEventInterface)
{
  delete m_TapiEventInterface;
  m_TapiEventInterface = NULL;
}

---

m_TapiEventInterface is a ptr to my implementation of the object sink, and m_pCP is also within my main class and not in the objects sink. So is this code still ok?

Yes, I think that is fine. Typically, you would control the lifetime of m_TapiEventInterface with reference counting rather than explicitly deleting it after the Unadvise:

Code:

---

class CTapiEventInterface : public ITTAPIEventNotification
{
private:
    UINT m_cRefs;

public:
  STDMETHODIMP QueryInterface(REFIID riid, void ** ppv)
  {
          if (ppv == NULL) return E_INVALIDARG;

          if (riid == IID_IUnknown || riid == IID_IDispatch || riid == IID_ITTAPIEventNotification))
          {
                  *ppv = (IUnknown *) this;
                  AddRef();
                  return S_OK;
          }
          *ppv = NULL;
          return E_NOINTERFACE;
  }

  STDMETHODIMP_(ULONG) AddRef(void)
  {
      return ++m_cRefs;
  }

  STDMETHODIMP_(ULONG) Release(void)
  {
      if (--m_cRefs == 0) /* Delete ourselves only when the reference count reaches zero. */
      {
          delete this;
          return 0;
      }
      return m_cRefs;
  }

...


};

---

However, your method of deleting the sink after calling Unadvise should be fine with well behaved objects.

I think it is extremely unlikely that Unadvise would fail. If you wanted to continue after Unadvise failing you would have to leave the sink alive, otherwise the next event call would crash your app.

So I do not need to delete the class explicitly? In my main class I create an instance of the sink with:

Code:

---

  m_TapiEventInterface = new CTAPIEventNotification();

---

However, if the sink deletes itself anyway I should not have to do this: delete m_TapiEventInterface? Is this correct?

Thanks.

Yes, that's right, if you get the reference counting working, it should delete itself internally when the reference count reaches zero. You can not delete an object twice, so you should not have delete m_TapiEventInterface elsewhere.

You may wish to temporarily put a MessageBox or breakpoint call in the event sink's destructor to make sure that it is destructing when you expect it to.

Yep, cheers. Thanks for your advice, Dene.