[geos-devel] Exception-safety (230KB)

[Mateusz Łoskot]

Hi,

Recently, I reported some problems about possible memory leaks that can
occur when Geometries construction is interrupted by exception.
Until now, I reported bugs #86 and #89 related to LineString class and
Point class. But I expect there are more similar places because current
approach of constructor implementation is a kind of "common pattern"
widely used in GEOS.

The problem applies only to situation when constructor throws an
exception (as in LineString or Point class). In this situation
destructor is never called and dynamically allocated resources, like
LineString::points sequence, are not deallocated.
Reference:
http://www.parashift.com/c++-faq-lite/exceptions.html#faq-17.4

So, I'd like to provoke small discussion about how to achieve
exception-safety in geos. IMHO, Exception-safety is pretty important
step to stability.

I see two obvious ways to promise exception safety
and avoid memory leaks (examples are a bit pseudocode):

1. Find unsafe places and put "delete XXX;" before every "throw"
instruction in constructors:

Foo::p_ - internal data

Foo::Foo(Bar* p) : p_(p)
{
   if (something)
   {
      delete p_; // For our safety
      throw std::exception();
   }
}

2. Second solution and, honestly, my favourite is to use smart pointers.
If Foo::p_ would be "a smart pointer" instead of raw pointer, then the
code is simplier and developer is not required to remember about all
those manual deallocations:

Foo::Foo(BarSmartPtr p) : p_(p)
{
   if (something)
      throw std::exception();
}

That's all.

I prepare small example program to test exceptions
not-safety and safety in real code.
I attached it to this post, file exception-safety-test.tar.gz
Inside the package, you will find README.txt file with short
instruction how to build it with VC++ and GCC.
NOTE: You don't need a Boost. I included Boost's smart pointers to the
package, so just read the README.txt and have fun :-)

There are two tests:

1) test_ctor_exception (presents unsafety of raw pointers)

------ test_ctor_exception - start ------
   noleak: Foo::Foo()
not-throwing: A::A()
not-throwing: A::~A()
   noleak: Foo::~Foo()

   bigleak: Foo::Foo()
throwing: A::A()
throwing: ctor says hello but dctor won't have any chance to say it!
We are NOT cool!
------ test_ctor_exception - end ------


PROBLEM: bigleak is not destroyed in the second case

2) test_ctor_exception_smart_ptr (presents how to get rid of unsafety
with shared_ptr).

------ test_ctor_exception_smart_ptr - start ------
   noleak1: Bar::Bar()
not-throwing: B::B()
not-throwing: B::~B()
   noleak1: Bar::~Bar()

   noleak2: Bar::Bar()
throwing: B::B()
throwing: ctor says hello AND dctors of B and Bar also say hello!
   noleak2: Bar::~Bar()
We are VERY cool!
------ test_ctor_exception_smart_ptr - end ------



Conclusion

Discussions about smart pointers and Boost appear and disappear from
time to time.
I think it's worth to take some decision.
I'd really recommend to start using "Smart Pointer" idioms.
Smart Pointers does not mean we should use Boost's smart pointers, we
can write our own simple smart pointers but the question is if it's
worth of the effort.
Boost's smart pointers are well tested and well working,
so why not to use them?

I'd summary my opinion as follows:
There should be no question if GEOS should use smart pointers or not,
because smart pointers can bring invaluable help; the only question I
see is which smart pointers implementation to use: Boost, our own, ...
Next, smart pointers can help not only to fix the problems
described in this post but much more.

NOTE: It is NOT required to build any Boost library to use smart
pointers. As you can see in the example I attached, they are just
simple headers.
We don't have to force users to install complete Boost.
We can extract every library we wish to use just as I did so with smart
pointers in the attached example. There is a special tool called bcp.
It is not perfect but works well. The only imperfection I know is that
it extracts quite big amount of files (e.g. for smart pointer, 2.7MB).
Doing it manually makes it possible to get much smaller package.
But I think the disc space is not a problem today :-)


One more Boost digression

Boost will be included in the C++ Library soon.
Recently, shared_ptr became a part of C++ Library Technical Report 1:
http://boost.org/libs/smart_ptr/shared_ptr.htm#Introduction

Cheers
-- 
Mateusz Łoskot
http://mateusz.loskot.net
-------------- next part --------------
A non-text attachment was scrubbed...
Name: exception-safety-test.tar.gz
Type: application/gzip
Size: 231941 bytes
Desc: not available
Url : http://lists.osgeo.org/pipermail/geos-devel/attachments/20060406/94e2a44c/exception-safety-test.tar.bin