serialization gives memory leaks due to throwing constructors
Dear all, I ran into an exception thrown by the constructor. But there is no way to call 'delete_created_pointers' on the object since the object is never fully created (see below) Is there something to do about? void Foo() { std::vector<Bus> v(2); //from the examples std::stringstream sstr; //save try { _ASSERT(sstr.good()); boost::archive::xml_oarchive oa(sstr); oa << BOOST_SERIALIZATION_NVP(v); } catch (boost::archive::archive_exception& re) { DBG_TRACEN(re.what()); } //load v.clear(); //sstr.seekg(0); sstr.seekg(1); //generate dileberate error boost::archive::xml_iarchive* p = NULL; try { _ASSERT(sstr.good()); boost::archive::xml_iarchive ia(sstr); p = &ia; ia >> BOOST_SERIALIZATION_NVP(v); } catch (boost::archive::archive_exception& re) { DBG_TRACEN(re.what()); //won't work: p is always NULL if (p) { p->delete_created_pointers(); } } } This throwing c++ constructor stuff is btw the way to transport errors according to a lot of c++ gurus. But my persnonal opinion is that it sucks. Every created object must be guarded with exception safety. For large applications, this exception mecahnism is very tricky and sources for a lot unwanted application terminations or memory leaks. Wkr me
Dear all, oops. My example never works... p is either NULL or pointing to a dead object. But the original question still stands. wkr, me
On 3/1/07 2:18 PM, "gast128"
Dear all,
I ran into an exception thrown by the constructor. But there is no way to call 'delete_created_pointers' on the object since the object is never fully created (see below)
Is there something to do about?
<snip>
This throwing c++ constructor stuff is btw the way to transport errors according to a lot of c++ gurus. But my persnonal opinion is that it sucks. Every created object must be guarded with exception safety. For large applications, this exception mecahnism is very tricky and sources for a lot unwanted application terminations or memory leaks.
I agree. At my workplace we have a convention. For heavyweight objects that would do interesting things in their constructors we instead use the alloc-init paradigm of Objective-C. In this model, the constructor does simply non-error things, then a corresponding init (or even suite of init methods) does the interesting things. This has the added benefit that the init method can be overridden in subclasses, and you don't need to re-implement all the behaviors. -Andrew
There are differing opinions on this. For example: http://www.arkestra.demon.co.uk/errors_cpp.html#acquire_resources_in_constru... and http://www.research.att.com/~bs/bs_faq2.html#ctor-exceptions Robert Ramey Andrew Mellinger wrote:
On 3/1/07 2:18 PM, "gast128"
wrote: Dear all,
I ran into an exception thrown by the constructor. But there is no way to call 'delete_created_pointers' on the object since the object is never fully created (see below)
Is there something to do about?
<snip>
This throwing c++ constructor stuff is btw the way to transport errors according to a lot of c++ gurus. But my persnonal opinion is that it sucks. Every created object must be guarded with exception safety. For large applications, this exception mecahnism is very tricky and sources for a lot unwanted application terminations or memory leaks.
I agree. At my workplace we have a convention. For heavyweight objects that would do interesting things in their constructors we instead use the alloc-init paradigm of Objective-C. In this model, the constructor does simply non-error things, then a corresponding init (or even suite of init methods) does the interesting things. This has the added benefit that the init method can be overridden in subclasses, and you don't need to re-implement all the behaviors.
-Andrew
Andrew Mellinger
This throwing c++ constructor stuff is btw the way to transport errors according to a lot of c++ gurus. But my persnonal opinion is that it sucks. Every created object must be guarded with exception safety.
I don't know what "guarded with exception safety" means. You certainly don't need try/catch blocks everywhere.
For large applications, this exception mecahnism is very tricky and sources for a lot unwanted application terminations or memory leaks.
Writing exception-safe code is not really any harder than writing code that's correct in the presence of errors with any other error-reporting mechanism. In fact, given a clear understanding of how to use exceptions effectively, they are less tricky to use correctly than other approaches to error handling, because they provide a high-level abstraction that lets you concentrate on what matters.
I agree. At my workplace we have a convention. For heavyweight objects that would do interesting things in their constructors we instead use the alloc-init paradigm of Objective-C. In this model, the constructor does simply non-error things, then a corresponding init (or even suite of init methods) does the interesting things. This has the added benefit that the init method can be overridden in subclasses, and you don't need to re-implement all the behaviors.
The alloc-init paradigm _really_ sucks. Instead of considering exceptions in your constructors you end up having to guard all the rest of the code with "what if this object failed its initialization?" checks/handlers. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams
I don't know what "guarded with exception safety" means. You certainly don't need try/catch blocks everywhere. ... Writing exception-safe code is not really any harder than writing code that's correct in the presence of errors with any other error-reporting mechanism. In fact, given a clear understanding of how to use exceptions effectively, they are less tricky to use correctly than other approaches to error handling, because they provide a high-level abstraction that lets you concentrate on what matters.
Hello David, my point is that exceptions are fatal when not dealt with. This can be a good thing in case of fatal exceptions (e.g. memory exhaustion), but bad when the exceptions are not that severe (e.g. string length on a NULL string). Another item is that I think that one of the rationales is that it is cleaner to write code like: void Foo() { try { //write logic } catch () { //write exception case(s) } } This throwing ctor stuff can be annoying if one considers the follwing case: void GetAllEmployees() { std::vector<Employee> v; Employee e = Load(...) v.push_back(e); //etc. } consider that one of the employees can 'become' corrupt. But still the rest of the employees should be dealt with. With exception handling it becomes: void GetAllEmployees() { std::vector<Employee> v; try { Employee e = Load(...) v.push_back(e); } catch () { } //etc. } No more nice seperation between logic and exception. wkr, me
gast128
Hello David,
my point is that exceptions are fatal when not dealt with. This can be a good thing in case of fatal exceptions (e.g. memory exhaustion),
Whether or not memory exhaustion should be fatal to a program is open to debate, but if you do need to shut the program down completely and quickly, exceptions are usually not the right mechanism to do it (http://groups.google.com/group/comp.lang.c++.moderated/browse_frm/thread/800...)
but bad when the exceptions are not that severe (e.g. string length on a NULL string).
If that's a programming bug, it should almost certainly *not* throw an exception.
Another item is that I think that one of the rationales is that it is cleaner to write code like:
void Foo() { try { //write logic } catch () { //write exception case(s) } }
Rationales, for what? Cleaner than what?
This throwing ctor stuff can be annoying if one considers the follwing case:
void GetAllEmployees() { std::vector<Employee> v;
Employee e = Load(...) v.push_back(e);
//etc. }
consider that one of the employees can 'become' corrupt.
It's almost impossible to write reasonable programs when you have to "consider that one of the xxx objects can 'become' corrupt." http://groups.google.com/group/comp.lang.c++.moderated/msg/659b9db50f587dab
But still the rest of the employees should be dealt with. With exception handling it becomes:
void GetAllEmployees() { std::vector<Employee> v;
try { Employee e = Load(...) v.push_back(e); } catch () { }
//etc. }
No more nice seperation between logic and exception.
I don't know why anyone would do that. Which employee has 'become' corrupt here, and how does the above code supposedly help? It seems to just eat exceptions and mask errors, nothing more. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams
Another item is that I think that one of the rationales is that it is cleaner to write code like:
void Foo() { try { //write logic } catch () { //write exception case(s) } }
Rationales, for what? Cleaner than what?
this is more or less my interpretation of chapter 14 of Stroustrup's book 'The C++ programming language'.
... It's almost impossible to write reasonable programs when you have to "consider that one of the xxx objects can 'become' corrupt." http://groups.google.com/group/comp.lang.c++.moderated/msg/659b9db50f587dab ... I don't know why anyone would do that. Which employee has 'become' corrupt here, and how does the above code supposedly help? It seems to just eat exceptions and mask errors, nothing more.
In a way it is. One can think of let the progam just continue its work, but a repair action must be scheduled the next time. This can be defensive or make things worse depending on the problem context. wkr, me
on Mon Mar 05 2007, gast128
David Abrahams
writes: Hello David,
Another item is that I think that one of the rationales is that it is cleaner to write code like:
void Foo() { try { //write logic } catch () { //write exception case(s) } }
Rationales, for what? Cleaner than what?
this is more or less my interpretation of chapter 14 of Stroustrup's book 'The C++ programming language'.
That fact doesn't seem to answer either question.
... It's almost impossible to write reasonable programs when you have to "consider that one of the xxx objects can 'become' corrupt." http://groups.google.com/group/comp.lang.c++.moderated/msg/659b9db50f587dab ... I don't know why anyone would do that. Which employee has 'become' corrupt here, and how does the above code supposedly help? It seems to just eat exceptions and mask errors, nothing more.
In a way it is. One can think of let the progam just continue its work, but a repair action must be scheduled the next time. This can be defensive or make things worse depending on the problem context.
IME defensive measures almost always make things worse. As noted in the thread I reference above, nobody has really developed a discipline that tells us what things to defend against, when to stop checking, and what we can reliably do when a problem is found. The result tends to be programs full of "corruption checks" and bogus "recovery code" that never gets tested or executed, making the program much harder to debug and maintain. In my experience, that approach vastly increases the likelihood of bugs. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams
Rationales, for what? Cleaner than what?
this is more or less my interpretation of chapter 14 of Stroustrup's book 'The C++ programming language'.
That fact doesn't seem to answer either question.
Maybe I may quote mr Stroustrup in his book: "The exception handling mechanism provides an alternative to the traditional techniques when they are insufficient, inelegant and error prone. It provides a way of explicitly separating error handling code from 'ordinary' code, thus making the program more readable and more emenable to tools" (etc.) I ran across a nice mail thread which a lots of pro's and cons about this matter: http://www.dbforums.com/archive/index.php/t-689599.html
... It's almost impossible to write reasonable programs when you have to "consider that one of the xxx objects can 'become' corrupt."
http://groups.google.com/group/comp.lang.c++.moderated/msg/659b9db50f587dab
... I don't know why anyone would do that. Which employee has 'become' corrupt here, and how does the above code supposedly help? It seems to just eat exceptions and mask errors, nothing more.
In a way it is. One can think of let the progam just continue its work, but a repair action must be scheduled the next time. This can be defensive or make things worse depending on the problem context.
IME defensive measures almost always make things worse. As noted in the thread I reference above, nobody has really developed a discipline that tells us what things to defend against, when to stop checking, and what we can reliably do when a problem is found. The result tends to be programs full of "corruption checks" and bogus "recovery code" that never gets tested or executed, making the program much harder to debug and maintain. In my experience, that approach vastly increases the likelihood of bugs.
I have quite the opposite opinion: I work on a new version of a large application and the old team had a very strict exception policy which gave the application instability and crashes. Because of this experience we built a much more forgiveness (without sacrifycing data integrity) in the application, which makes it more robust. This does of course not mean that every pointer is checked, but an example can be that subsystems must check their arguments before continuing their work. Incorrect arguments will not be signaled by exceptions. wkr, me I should one day give my real name..
on Wed Mar 07 2007, gast128
David Abrahams
writes: IME defensive measures almost always make things worse. As noted in the thread I reference above, nobody has really developed a discipline that tells us what things to defend against, when to stop checking, and what we can reliably do when a problem is found. The result tends to be programs full of "corruption checks" and bogus "recovery code" that never gets tested or executed, making the program much harder to debug and maintain. In my experience, that approach vastly increases the likelihood of bugs.
I have quite the opposite opinion: I work on a new version of a large application and the old team had a very strict exception policy which gave the application instability and crashes.
Having a very _strict_ exception policy is no help at all if you pick the _wrong_ exception policy.
Because of this experience we built a much more forgiveness (without sacrifycing data integrity) in the application, which makes it more robust.
How do you know it isn't hiding prorgam bugs?
This does of course not mean that every pointer is checked, but an example can be that subsystems must check their arguments before continuing their work. Incorrect arguments will not be signaled by exceptions.
You don't seem to be reading the thread I referenced. Signalling incorrect arguments with exceptions is totally contrary to what I advocate. -- Dave Abrahams Boost Consulting www.boost-consulting.com
I agree. At my workplace we have a convention. For heavyweight objects that would do interesting things in their constructors we instead use the alloc-init paradigm of Objective-C. In this model, the constructor does simply non-error things, then a corresponding init (or even suite of init methods) does the interesting things. This has the added benefit that the init method can be overridden in subclasses, and you don't need to re-implement all the behaviors.
The alloc-init paradigm _really_ sucks. Instead of considering exceptions in your constructors you end up having to guard all the rest of the code with "what if this object failed its initialization?" checks/handlers.
I wouldn't use that strong words. At least you have the advantage to use virtual functions which are not behaving like virtuals in constructors as the orginal poster describes. Buschmann describes in 'posa' the use of MVC in this way.
on Thu Mar 15 2007, gast128
I agree. At my workplace we have a convention. For heavyweight objects that would do interesting things in their constructors we instead use the alloc-init paradigm of Objective-C. In this model, the constructor does simply non-error things, then a corresponding init (or even suite of init methods) does the interesting things. This has the added benefit that the init method can be overridden in subclasses, and you don't need to re-implement all the behaviors.
The alloc-init paradigm _really_ sucks. Instead of considering exceptions in your constructors you end up having to guard all the rest of the code with "what if this object failed its initialization?" checks/handlers.
I wouldn't use that strong words. At least you have the advantage to use virtual functions which are not behaving like virtuals in constructors as the orginal poster describes. Buschmann describes in 'posa' the use of MVC in this way.
That's a small convenience, true, but it's hardly worth weakening your class invariants to get it. -- Dave Abrahams Boost Consulting www.boost-consulting.com
On 3/15/07 2:05 PM, "David Abrahams"
on Thu Mar 15 2007, gast128
wrote: I agree. At my workplace we have a convention. For heavyweight objects that would do interesting things in their constructors we instead use the alloc-init paradigm of Objective-C. In this model, the constructor does simply non-error things, then a corresponding init (or even suite of init methods) does the interesting things. This has the added benefit that the init method can be overridden in subclasses, and you don't need to re-implement all the behaviors.
The alloc-init paradigm _really_ sucks. Instead of considering exceptions in your constructors you end up having to guard all the rest of the code with "what if this object failed its initialization?" checks/handlers.
I wouldn't use that strong words. At least you have the advantage to use virtual functions which are not behaving like virtuals in constructors as the orginal poster describes. Buschmann describes in 'posa' the use of MVC in this way.
That's a small convenience, true, but it's hardly worth weakening your class invariants to get it.
I am surprised that my original comment has caused such a strong response. In my experience there are many ways to solve a problem and there isn't a silver bullet. We see benefits to alloc-init paradigm for certain classes that outweigh the construction method. On others we don't do that. I am surprised (and disappointed) that you choose to lump all uses of the alloc-init as *_really_ sucks* without understanding our use case. -Andrew
gast128 wrote:
Dear all,
I ran into an exception thrown by the constructor. But there is no way to call 'delete_created_pointers' on the object since the object is never fully created (see below)
Note: the function of 'delete_created_pointers' is to delete any pointer created during the process of loading. Construction of an archive creates no such pointers. So not calling 'delete_created_pointers' in this case will have no effect and does not need to be called. Robert Ramey
Robert Ramey
Note: the function of 'delete_created_pointers' is to delete any pointer created during the process of loading. Construction of an archive creates no such pointers. So not calling 'delete_created_pointers' in this case will have no effect and does not need to be called.
Ok 3 things: 1) I think a throwing ctor should not create memory leaks. I believe Sutter has comments about this in his book 2) The loaded XML file can be altered by users. Therfore the load can fail and I want it to be guarded. But now I have to add a double guard: bool Load(const std::string& cr) { std::fstream fstr(cr); try { boost::archive::xml_iarchive ia(fstr); try { ia >> ...; return true; } catch (boost::archive::archive_exception& re) { ia.delete_created_pointers(); } } catch (boost::archive::archive_exception& re) { } return false; } 3) I am aware of this 'exception versus error returning' discussions. A good article is written by Sutter in cuj august 2004 'When and How to Use Exceptions'. But I get the impression that this is a discussion between academic correct code versus common practice. In our company we have the policy that subsystems should not let escape exceptions. An unimportant class should not terminate the application because an exception is not caught. The GUI layer should not terminate the application because it cannot display all its elements. I would suggest for those c++ gurus to take software architecture in consideration when requiring this exception stuff for all classes. If you follow their rules all the time it takes twice the effort to code applications. And it will probably terminate a lot more due to uncaught exceptions from the 80% classes which are mildly important. Wkr, me
I don't think this case needs two layers of try/catch. This thread started with a concern about throwing an exception while in a constructor. If an exception is thrown during construction, there are no pointers created. So calling "delete created pointers" is a no-op and does no harm if it is called. If an exception is thrown during loading, then "delete_created_pointers" will becalled with the desired effect. This is not to say that I know for a fact that that an exception thrown during construction can in fact create a memory leak. To be honest, I never considered that when I wrote the code so it might be possible. I would have to look into it. Robert Ramey gast128 wrote:
Robert Ramey
writes: Note: the function of 'delete_created_pointers' is to delete any pointer created during the process of loading. Construction of an archive creates no such pointers. So not calling 'delete_created_pointers' in this case will have no effect and does not need to be called.
Ok 3 things: 1) I think a throwing ctor should not create memory leaks. I believe Sutter has comments about this in his book 2) The loaded XML file can be altered by users. Therfore the load can fail and I want it to be guarded. But now I have to add a double guard:
bool Load(const std::string& cr) { std::fstream fstr(cr);
try { boost::archive::xml_iarchive ia(fstr);
try { ia >> ...; return true; } catch (boost::archive::archive_exception& re) { ia.delete_created_pointers(); } } catch (boost::archive::archive_exception& re) { }
return false; }
"Robert Ramey"
This is not to say that I know for a fact that that an exception thrown during construction can in fact create a memory leak. To be honest, I never considered that when I wrote the code so it might be possible. I would have to look into it.
It's almost impossible to make a mistake with that if you follow Peter Dimov's advice that every newly allocated resource be immediately managed by a _named_ resource manager object. -- Dave Abrahams Boost Consulting www.boost-consulting.com
On 3/1/07, Robert Ramey
Note: the function of 'delete_created_pointers' is to delete any pointer created during the process of loading. Construction of an archive creates no such pointers. So not calling 'delete_created_pointers' in this case will have no effect and does not need to be called.
Hi Since I throw an exception in "load", following your earlier advice (conditional loading), I wonder if I should call delete_created_pointers or the archives' destructors make that redundant. Thanks!
n.torrey.pines@gmail.com wrote:
On 3/1/07, Robert Ramey
wrote: Note: the function of 'delete_created_pointers' is to delete any pointer created during the process of loading. Construction of an archive creates no such pointers. So not calling 'delete_created_pointers' in this case will have no effect and does not need to be called.
Hi
Since I throw an exception in "load", following your earlier advice (conditional loading), I wonder if I should call delete_created_pointers or the archives' destructors make that redundant.
The archive destructors don't do this. I thought it might be presumptuous. To my mind, the best solution is to use smart_pointers and serialize them. Robert Ramey
Robert Ramey
To my mind, the best solution is to use smart_pointers and serialize them.
Hello Robert, this gives me another problem (crash) when using an object consisting of two shared pointers: struct Bla { template <class Archive> void serialize(Archive& ar, const unsigned int /*version*/) { ar & BOOST_SERIALIZATION_NVP(m_ptr); ar & BOOST_SERIALIZATION_NVP(m_ptr1); } boost::shared_ptr<int> m_ptr; boost::shared_ptr<int> m_ptr1; }; If I try to load this, and let if fail while loading the second shared pointer (because the file is corrupt) it crashes when using the 'delete_created_pointers' option in the catch handler due to the fact that the shared pointer wants to cleanup an already deleted pointer. Leaving this option out does not crash it anymore, but results in some memory leaks. But maybe I am taking now the serialziation library beyond its design limits. These are all of course exceptional case. wkr, me
I don't see any problem with the code snippet. Maybe you want to make small test which we run here. Robert Ramey gast128 wrote:
Robert Ramey
writes: To my mind, the best solution is to use smart_pointers and serialize them.
Hello Robert,
this gives me another problem (crash) when using an object consisting of two shared pointers:
struct Bla { template <class Archive> void serialize(Archive& ar, const unsigned int /*version*/) { ar & BOOST_SERIALIZATION_NVP(m_ptr); ar & BOOST_SERIALIZATION_NVP(m_ptr1); }
boost::shared_ptr<int> m_ptr; boost::shared_ptr<int> m_ptr1; };
If I try to load this, and let if fail while loading the second shared pointer (because the file is corrupt) it crashes when using the 'delete_created_pointers' option in the catch handler due to the fact that the shared pointer wants to cleanup an already deleted pointer. Leaving this option out does not crash it anymore, but results in some memory leaks.
But maybe I am taking now the serialziation library beyond its design limits. These are all of course exceptional case.
wkr, me
Robert Ramey
I don't see any problem with the code snippet. Maybe you want to make small test which we run here.
ok here it comes. It looks artificial, but it is just a simplification of a
case. It gives an access violation after thhe load, when it tries to destroy
the object.
#include
gast128 wrote:
Robert Ramey
writes: I don't see any problem with the code snippet. Maybe you want to make small test which we run here.
ok here it comes. It looks artificial, but it is just a simplification of a case. It gives an access violation after thhe load, when it tries to destroy the object.
OK - here are the changes I made in order for your example to work:
the main one is to enclose the output in {} to be sure that the xml
archive dtor is called. The ensures that the xml file contains the
trailer tags. This was what calling the load exception. I discovered
this by visually inspecting the xml file to and it was obvious to see
that it wasn't terminated correctly.
I just commented out some stuff that made no sense to me.
Robert Ramey
int main()
{
BoostExample obj;
std::stringstream sstr;
//save make sure dtor is called on the oa
// by enclosing it in {}
{
boost::archive::xml_oarchive oa(sstr);
oa << BOOST_SERIALIZATION_NVP(obj);
}
//triggers a load exception
// what is this for?
#if 0
std::string str = sstr.str();
boost::replace_all(str, "
OK - here are the changes I made in order for your example to work:
the main one is to enclose the output in {} to be sure that the xml archive dtor is called. Yes I forget that when I ported the example to std formats instead of my own
Robert Ramey
I just commented out some stuff that made no sense to me.
Robert Ramey
//triggers a load exception // what is this for? #if 0 std::string str = sstr.str(); boost::replace_all(str, "
", "
- the reset's are for clearing the object (not necessary) - with seekg(0) I hope that the loading starts reading at the begin of the stream (where the XML starts) - the replace stuff was a simulation of an actual case: the second shared pointer entry in the XML became corrupt, which triggered an excpetion. However the first shared pointer seems to be loaded complete already. Calling 'delete_created_pointers' on the archive freed this piece of memory also, so that an access violation is triggered when the object (and thus the shared pointer) went out of scope. Wkr, me
participants (5)
-
Andrew Mellinger
-
David Abrahams
-
gast128
-
n.torrey.pines@gmail.com
-
Robert Ramey