Re: [boost] Stronger exception safety guarantee for smart_ptr.reset()

From: Vesa Karvonen (vesa.karvonen_at_[hidden])
Date: 2001-06-18 09:17:43

Note: I have combined the two separate messages from David Abrahams and John
Maddock into one, so that I can answer both of their comments at the same
time.

From: "David Abrahams" <david.abrahams_at_[hidden]>
> From: "Vesa Karvonen" <vesa.karvonen_at_[hidden]>
> >
> > In the C++ standard library, as well as in Boost, there is a requirement
> > that objects may not throw in destructors. Unfortunately this is very
> > difficult to ensure both in theory and in practice
>
> Not in my experience, FWIW.
>
> > , because a destructor
> > may involve an arbitrarily complex computation and may, in particular,
> > call functions that are not destructors.
>
> Most destructor bodies I write, however, are empty. I think that has become
> an expectation among modern C++ programmers. It's fairly easy to follow the
> rule, "if you write a non-empty destructor body, consider what exceptions
> may be thrown".

From: "John Maddock" <John_Maddock_at_[hidden]>
> Come to that what does it mean for a destructor to throw anyway?

Throwing in a destructor is indeed bad practice and should be discouraged.

However, sometimes a destructor is the most convenient place to call other
functions for their side-effects. Typical examples are logging and tracing,
but there are other interesting situations. Such practice may not be award
winning, but it is often more convenient than moving the code out of the
destructor or using try-catch blocks in every destructor.

Ensuring the exception safety of every destructor is a rather involved task.
In particular, much of the code may be legacy code that was written before the
issues of exception safety were well understood. In such a case, applying the
well known rule outlined above may not be easy.

From: "John Maddock" <John_Maddock_at_[hidden]>
> >The deletion of the newly allocated object can be guaranteed easily by
> using a variation of the swap-idiom for exception safety:
>
>   template<class T>
>   void auto_ptr<T>::reset(T* new_p)
>   { // short circuit self-assignment
>     if (this_p == new_p)
>       return;
>
>     // swap (performed here only partially)
>     T* old_p = this_p;
>     this_p = new_p;
>
>     // perform operations that might throw
>     delete old_p;
>   }
> <
>
> I don't think that that is any safer than the current version - all you've
> done is change the thing that leaks - now if the destructor throws it's
> old_ptr that leaks rather then new_ptr, how is this any better?

Not exactly. One significant problem is that it is unsafe to try to delete the
old object again. Deletion isn't typically reversible. This means that there
isn't much that can be done about leaking the old object. In fact, the current
practice:

  template<class T>
  void auto_ptr<T>::reset(T* new_p)
  { if (this_p == new_p)
      return;

    delete this_p;

    this_p = new_p;
  }

Is arguably unsafe, because it leaves the old pointer intact and may result in
deleting the pointer twice. (Yes, I know that auto_ptr::reset() has exception
specification. More about that later.)

On the other hand, it should be safe to try to delete the new object.

(It is also possible to release() the old pointer before deletion, thus
preventing double deletion.)

From: "David Abrahams" <david.abrahams_at_[hidden]>
> From: "Vesa Karvonen" <vesa.karvonen_at_[hidden]>
> > Consider the following code:
> >
> >   auto_ptr<may_throw> ap(new may_throw);
> >
> >   ap.reset(new may_throw);
> >
> > According to the C++ standard, the behavior of the reset() operation in
> > the above code snippet is undefined if deletion of the may_throw object
> > throws an exception. In particular, the newly allocated object, to which
> > there is no pointer anymore, may or may not get assigned to the auto_ptr
> > and therefore might become a memory leak.
> >
> > A stronger guarantee, in which the assignment of the pointer to the smart
> > pointer would be guaranteed, would be arguably safer. It is also important
> > to note, that such a guarantee would not sacrifice any current behavior,
> > because the behavior is currently undefined.
>
> >From a standards POV, I think this change would be a mistake.
> auto_ptr<T>::reset() is currently guaranteed not to throw an exception.

I'm not entirely convinced that this is true. Perhaps I'm missing a critical
piece of knowledge.

Please keep in mind that the behavior of the code is undefined if the deletion
throws - at least, this is my current interpretation based on 17.4.3.6 and
20.4.5. Undefined behavior, according to my interpretation, is a stronger
specification that effectively overrides exception specifications.

Also keep in mind, that the Boost smart pointer nothrow guarantee is, both in
practice and in theory, rather weak, because Boost smart pointers do not use
exception specifications.

((I do not know whether the overhead of exception specifications can be
removed (in theory). Judging from the pace of compiler development, it could
take a long time before such compiler technology would be available. Currently
the overhead of a fully standards conforming auto_ptr, that includes the
exception specifications, is significant enough to steer away from using it.))

The above points can be summarized using the following table:

    Current run-time behavior of smart pointers

                   | with excpt. spec   | without excpt. spec
  -----------------+--------------------+--------------------
  deletion throws  | undefined behavior | undefined behavior
  -----------------+--------------------+--------------------
  deletion nothrow | nothrow behavior   | nothrow behavior
  -----------------+--------------------+--------------------

I think that the current nothrow guarantee of smart pointer reset() (including
std::auto_ptr and boost::shared_ptr) gives a false sense of exception safety,
because it simply doesn't guarantee any consistent programmatically detectable
effect - in the case that an exception is actually throw.

From: "David Abrahams" <david.abrahams_at_[hidden]>
> Unless you remove that guarantee or swallow exceptions thrown by T (either
> one IMO a very bad idea), it is meaningless to talk about what happens if T
> throws.

Exceptions are an interesting topic and I don't like defeatism. I'm very
interested to know about any practical problems with the practice I have
presented and also about specific pros and cons compared to the current
practice.

From: "David Abrahams" <david.abrahams_at_[hidden]>
> From: "Vesa Karvonen" <vesa.karvonen_at_[hidden]>
> > The deletion of the newly allocated object can be guaranteed easily by
> > using a variation of the swap-idiom for exception safety:
> >
> >   template<class T>
> >   void auto_ptr<T>::reset(T* new_p)
> >   { // short circuit self-assignment
> >     if (this_p == new_p)
> >       return;
> >
> >     // swap (performed here only partially)
> >     T* old_p = this_p;
> >     this_p = new_p;
> >
> >     // perform operations that might throw
> >     delete old_p;
> >   }
> >
> > The run-time overhead of the above technique, compared to the traditional
> > implementation, should be for all intents and purposes negligible.
> >
> > The above technique can be generalized to other smart pointers and more
> > general resources management primitives.
>
> >From an implementation POV, I'm not /opposed/ to the above as a way of
> providing better quality... but I'm not convinced it adds quality without
> accompanying guarantees from the standard. Since I don't think we can/should
> make those, for reasons previously outlined, I am at best lukewarm to this
> idea.

As I have demonstrated, the technique that I have described:
- Prevents double deletion of the old pointer.
- Prevents leaking the new pointer.
- Removes the nothrow exception specification (which is practically useless
anyway).

I think that the above features are desirable and in the light of my present
knowledge, I think that they are an improvement over current practice.