Re: [boost] [review] Review of Boost.Exception

From: Dean Michael Berris (mikhailberis_at_[hidden])
Date: 2007-10-01 01:01:26

On 9/30/07, Emil Dotchevski <emil_at_[hidden]> wrote:
> > > <snip>
> > > Sure, there are use cases when you know an error_info must be present
> > > in the exception or you have a bug, but in that case it makes more
> > > sense to assert on get_error_info.
> > >
> >
> > Far from it. I'm thinking get_error_info(T const &) should not compile
> > if the error_info being requested has not been passed to the
> > exception.
>
> I don't think this is possible. Consider that a main design goal of
> boost::exception is to be able to do this:
>
> try
> {
>     ---something---
> }
> catch( boost::exception & x )
> {
>     x << error_info<my_info>(....);
>     throw;
> }
>
> In general, when we finally catch the exception object, it may have
> my_info or not.
>

True, I'm sorry about the mental fart about it not compiling.
Sebastian has already confirmed my suspicions and I don't think it
would be practical to do this either.

> > At any rate, perhaps using something like Boost.Optional as the return
> > value (or an additional parameter to get_error_info) mitigates the
> > problem you describe. Somehow, for me the following reads a bit more
> > coherent if not safer:
> >
> > try {
> >   ...
> > } catch (boost::exception & e) {
> >   boost::optional<int> errno = boost::get_error_info<tag::errno>(e);
> >   if (errno) { // means errno is set
> >     cerr << *errno;
> >   } else { // means errno is not set, then set it
> >     e << boost::error_info<tag::errno>(-1);
> >   };
> >   throw; // propagate exception
> > };
>
> Would you agree that this is simpler and easier to read?:
>
> try {
>   ...
> } catch (boost::exception & e) {
>   if( int const * ec = boost::get_error_info<tag_errno>(e) ) {
>     cerr << *ec;
>   } else {
>      e << boost::error_info<tag::errno>(-1);
>   };
>   throw; // propagate exception
> };
>

Personally, I wouldn't agree that this is simpler because of personal
dislike for pointers, and how they can be mangled inadvertently.
Consider the bug that can be introduced by pointer semantics:

try { ... }
catch (boost::exception & e) {
  if ( int const * ec = boost::get_error_info<tag::errno>(e) ) {
    cerr << ec + 1;
  } else {
    e << boost::error_info<tag::errno>(-1);
  };
  throw; // propagate exception
};

Consider having a boost::optional<int> in place of `int const * ec`,
you don't run into this problem because adding 1 to an optional<int>
won't compile. FWIW, boost::optional<int> ec can also be put inside
the if condition so that it's available only inside the body of the if
implementation. This also won't expose a memory address which someone
can mangle with, not that it would make much sense doing that -- the
fact that it's possible makes it inherently dangerous.

> > <snip>
> > It's good to know that you haven't had any performance-related issues
> > with this library. I'll take your word for it.
> >
> > However, a shared_ptr<> implies allocating something on the heap.
> >
> > Anytime you make an (arguably) unnecessary memory allocation in the
> > heap, in my book is a performance affecting issue. We can go on and
> > argue about long jumps, cache misses, and memory locality issues, but
> > this would be moot since the goal of the library is primarily to
> > provide a functionality that seems to require the heap allocation *by
> > design*.
>
> In the particular case of Boost Exception, any speed overhead it adds
> should be compared to the overhead of throwing an exception. You have
> stack unwinding and destructor calls, etc. anyway. I don't think that
> optimizing Boost Exception for speed makes any sense.
>

Speed not entirely, true. Memory efficiency and memory-related
performance effects can arguably be reduced -- it's just a matter of
how much effort you want to put or not put into it, for admittedly
very little gain.

> Optimizing the memory allocations made by Boost Exception when info is
> added to exception objects can not be dismissed as easily, but I would
> not call those allocations unnecessary. :)

Yes. What I meant though is that there should be a way to remove if
not minimize these heap allocations into stack-allocated values
encapsulated in the exception. How this can be done is still hazy to
me, but I'm thinking it may have something to do with encapsulating
the exception into another wrapping exception which is thrown instead
of just letting the exception propagate in the natural scheme of
things. This may be contrary to the original design goals, so I'm not
sure how attractive that option is to you. :)

>
> Consider that all of these allocations happen immediately before or
> during stack unwinding, and at that time the rest of the code doesn't
> usually allocate memory. When the exception is finally handled, all
> this memory is reclaimed, and assuming that no other memory was
> allocated during the stack unwinding, we don't get fragmentation
> either.
>

I'm not sure I understand this, but without using a object pool
implementation like Boost.Pool that guarantees that areas in memory
allocated are contiguous or at least minimizes the risk of
fragmentation, I don't think it's safe to assume you don't get
fragmentation during stack unwinding and/or allocating heap space
within catch blocks.

> > > > My concern with the use of shared_ptr<>'s is that in cases where an
> > > > exception pertaining to "out of memory" conditions would arise,
> > > > throwing a boost::exception and then having to allocate more memory
> > > > just to encapsulate additional information would lead to undesirable
> > > > termination.
> > >
> > > Could you elaborate a bit more? Can you come up with a theoretical
> > > example that demonstrates your concerns?
> > >
> >
> > Theoretically, (or I think even in reality) a call to the (default)
> > new operator may trigger a bad_alloc exception. This exception may be
> > caught, and somewhere along the way code decides to encapsulate the
> > information in that bad_alloc exception into a boost::exception -- and
> > chooses to say where that exception happened, as a tag in the
> > propagated boost::exception.
>
> I guess I should have been more specific, could you elaborate how
> using boost::exception would "lead to undesirable termination"?
>

try {
  try {
    ...
  } catch (std::bad_alloc & e) {
    throw boost::exception() << error_info<tag::long_double>(0xFF);
  };
  ...
} catch (boost::exception & e) {
  e << error_info<tag::long_double_still>(0xFF);
  throw;
};

Unless you're catching bad_alloc explicitly outside of the outer
try-catch block, and unless bad_alloc derives from boost::exception,
then the possibility of throwing a bad_alloc while adding
error_info<tag::long_double_still>(0xFF) exists because you can run
out of memory while allocating from the heap. Without a handler for
bad_alloc outside the catch block, you're bound to get termination.

Admittedly this example is contrived, but may be really hard to trace
once the shared_ptr<>'s allocated when adding error_info objects
decides to throw a bad_alloc exception.

-- 
Dean Michael C. Berris
Software Engineer, Friendster, Inc.
[http://cplusplus-soup.blogspot.com/]
[mikhailberis_at_[hidden]]
[[phone]]
[[phone]]