Improve exception safety with smart pointers

[Markus Elfring]

Would you like to wrap any pointer data members with the template class "std::unique_ptr"?
http://en.wikipedia.org/wiki/Smart_pointer#unique_ptr

Update candidates:
* https://github.com/worldforge/ember/blob/f13863c8139fb03207b069867b9883f7d4e7bd0c/src/services/EmberServices.cpp#L98
* https://github.com/worldforge/ember/blob/67b7bc7569e631ecad228c00fe21e2a5abf9474a/src/components/ogre/environment/meshtree/TParameters.cpp#L57
* https://github.com/worldforge/ember/blob/efffaf9d1fa13066c9f939201263d2ea897b52ac/src/components/ogre/World.h#L266

[Erik Ogenvik (erik-ogenvik)]

We already do this in a lot of places, not just in all.
If there's already a fully functioning "delete" statement in the destructor I don't see much gained from altering it to unique_ptr.

[Markus Elfring (elfring)]

Would you like to delegate the responsibility for object deletion to more dedicated classes?
Would it be useful to get rid of explicit delete calls in your own source code?

How do you think about information from sources like the following?

* http://smart-pointers.sourceforge.net/#Description

* FAQ "How should I handle resources if my constructors may throw exceptions?"
  http://www.parashift.com/c++-faq/selfcleaning-members.html

* Answers for the question "Is memory allocated for an object automatically deleted if an exception is thrown in a constructor?"
  http://stackoverflow.com/questions/4717656/is-memory-allocated-for-an-object-automatically-deleted-if-an-exception-is-throw

* Yonat Sharon:
  http://ootips.org/yonat/4dev/smart-pointers.html#WhyExceptions

* Article "Constructor Exceptions in C++, C#, and Java" by Herb Sutter
  http://herbsutter.com/2008/07/25/constructor-exceptions-in-c-c-and-java/

[Erik Ogenvik (erik-ogenvik)]

Thanks for the links. I'm already well aware of how unique_ptr work though, and we do use them in many places.

What is it that you're proposing exactly?

[Markus Elfring (elfring)]

It is unsafe to assign an pointer that is returned by the new operator directly because the target variable was eventually not initialised with a well-known value like "nullptr" before a C++ exception would occur. So I suggest to improve the remaining source code places.

I'm sorry that I was not aware that a class like "unique_ptr" has got some uses in your code base already. I would appreciate if its usage can be completed.

[Erik Ogenvik (erik-ogenvik)]

That's not correct. It's always safe to assign a pointer that's returned by the new operator. In fact, that's one of the guarantees that are given: if the new operator returns you have a valid instance.

What you are thinking of is probably the case when there's an uncaught exception. In that case the new operator won't return; it will instead throw an exception.

Now, unique_ptr is useful when handing the case of there being an uncaught exception thrown from within a constructor. However, it's no panacea or universal fix, since you must also actually handle the exception somewhere else in order for it to actually matter.
So if we take EmberServices for example. Any _uncaught_ exception thrown from within the constructor of either EmberServices or any of the sub-services constitutes a catastrophic failure which puts the process in an invalid state, from where there's no recovery. Process shutdown is the only possible course. And since that's happening there's no real reason to try to free resources; the process is shutting down anyway.

I agree that there are many places throughout the code where unique_ptr would be more suitable than raw pointers. But absolutely not in all places; it totally depends on the specific object in question.

[Markus Elfring (elfring)]

I try to point out this special case once more: Would the target variable ever receive a valid pointer if a C++ exception was thrown during the processing of the new operator?

How do you think about to reduce the risks from uninitialised variables?

The class "EmberServices" might be a bad example for recovery attempts. I imagine that there are others where updates will be more useful, won't it?

I would have got some difficulties to distinguish the status of your own software from components which belong to other sources.

[Erik Ogenvik (erik-ogenvik)]

When talking about these technical things, try to be more precise in your wording. It's easy to misunderstand each other if we're not on the same page.

Are you asking if a variable or field can be correctly assigned with a valid pointer to a fully constructed instance, if there's an exception thrown in any of the constructors that are executed as a result of calling "new", and this exception isn't caught?
Or are you referring to another scenario than what I just described?

[Markus Elfring (elfring)]

Yes to the first question from the reply on 2014-03-02.

What is your expectation?
Can a C++ exception have got the effect that a desired pointer assignment will not be performed?

[Erik Ogenvik (erik-ogenvik)]

If there's an exception thrown while an instance is constructed through a call to "new", this exception will unwind the stack and the "new" call will never terminate. Thus no field or variable will be assigned anything.

You talk about a risk from uninitialized variables. But you must understand that there are very few cases, if any, in our code where this could even happen (though I can't speak much about some of the third party code that's part of the Ember source code).
See, when an exception is thrown it's either handled locally, because it was somewhat expected, through a try...catch statement, or it's not. In the case that it's not it's in almost all cases because there's no real simple way to handle it. Mainly because the exception puts the application in an invalid state. What we then do is to let the exception percolate upwards to a place where the subsystem from where it originated can be terminated, thus putting the application back in a valid state. If that's not possible (like with what I mentioned about exceptions in the EmberServices constructor) the whole process is shut down.

Could you point to any place in the source code where there would be an invalid state in a field if there was an exception thrown?

[Markus Elfring (elfring)]

Are the classes "TParameters" and "World" affected besides "EmberServices" for example?

[Erik Ogenvik (erik-ogenvik)]

Please explain the flow which could lead to an invalid state in any of those classes. Use example code.
Because I can't see it.

And try to be clear and precise when talking about these technical issues. Don't answer my request for an example with a vague question of your own.

[Markus Elfring (elfring)]

Would the following source code example need any software updates?

> X=experiment.cpp && cat $X && g++ $X && echo '---' && ./a.out
#include <iostream>
#include <stdexcept>

struct ball
{
 ball()
 {
  throw std::runtime_error("Surprise!");
 }
};

class game
{
 ball* bounce;

public:
 game()
 {
  std::cout << "Game ball 1: " << static_cast<void*>(bounce) << std::endl;
  bounce = new ball;
  std::cout << "Game ball 2: " << static_cast<void*>(bounce) << std::endl;
 }

 ~game()
 {
  std::cout << "Bye" << std::endl;
  delete bounce;
 }

 void play(void)
 {
  std::cout << "Action!" << std::endl;
 }
};

int main(void)
{
 std::cout << "Test started ..." << std::endl;
 game x;
 x.play();
}
---
Test started ...
Game ball 1: 0x4010e0
terminate called after throwing an instance of 'std::runtime_error'
  what(): Surprise!

[Erik Ogenvik (erik-ogenvik)]

I don't understand what you're trying to prove with this snippet. I asked you for an example to validate your assertion that "It is unsafe to assign an pointer that is returned by the new operator directly because the target variable was eventually not initialised with a well-known value like "nullptr" before a C++ exception would occur."
The code you posted on the contrary actually proves that your claim is incorrect.
Since the stack is unwound while creating "x" there will never be a valid "x" instance, and thus there will never be a "bounce" field. Thus there's no risk of ever having to worry about whether "bounce" is valid. It's a binary situation: either no exception is thrown in the constructor, and "bounce" points to a valid instance. Or there is an exception thrown and there's never any valid "x" instance to begin with, and thus never any "bounce" field either.

I agree that unique_ptr is useful for making sure that nested instance memory is correctly released in the event of an exception in a constructor. However, on your assertion that it's unsafe to assign a pointer that is returned by the new operator you must understand that you are incorrect about the memory model and program flow of C++.

[Markus Elfring (elfring)]

Does the following source code example become a bit more interesting after the addition of two pointer member variables?

> X=experiment.cpp && cat $X && g++ $X && echo '---' && ./a.out
#include <iostream>
#include <stdexcept>

struct ball
{
 char const* name;

 ball()
 {
  throw std::runtime_error("Surprise!");
 }

 ball(char const* tag) : name(tag)
 {
 }
};

class game
{
 ball *bounce, *jump;

public:
 game()
 {
  std::cout << "Game ball 1: " << static_cast<void*>(bounce) << std::endl;
  bounce = new ball("success");
  std::cout << "Game ball 2: " << static_cast<void*>(bounce) << std::endl;
  std::cout << "Game ball 3: " << static_cast<void*>(jump) << std::endl;
  jump = new ball;
  std::cout << "Game ball 4: " << static_cast<void*>(jump) << std::endl;
 }

 ~game()
 {
  delete jump;
  std::cout << "Bye" << std::endl;
  delete bounce;
 }

 void play(void)
 {
  std::cout << "Action!" << std::endl;
 }
};

int main(void)
{
 std::cout << "Test started ..." << std::endl;
 game x;
 x.play();
}
---
Test started ...
Game ball 1: 0x401190
Game ball 2: 0x1fd1010
Game ball 3: 0x400d70
terminate called after throwing an instance of 'std::runtime_error'
  what(): Surprise!

Would it be useful to release the object "bounce" correctly?

[Erik Ogenvik (erik-ogenvik)]

Ok, I'm closing this issue since you're obviously not listening to a word I'm saying.

Yes, as I've said multiple times unique_ptr can help with _memory leaks_ caused by exceptions thrown in the constructor. No one is arguing against that.
However, as I've also stated you must also be able to correctly recover from an unexpected exception for there to be any benefit. If the process has to be shut down anyway there's not much gained.

What you're wrong about is however your assertion that it's unsafe to assign a pointer returned from a new statement, because you somehow think that the pointer returned could be invalid. This is simply not the case, and you've repeatedly now posted code which disproves you.

[Markus Elfring (elfring)]

I suggest to adjust the perspective then. Does any object (or memory) which was not appropriately released contain valuable information?

Can it be that "a second attempt" might succeed for recovery from a thrown exception?

[Markus Elfring (elfring)]

I would like to add another view on such implementation details.

My small source code examples show only that specific destructors were not called after a exception was thrown. The tested behaviour fits to rules of the C++ programming language.
http://www.parashift.com/c++-faq/overview-dtors.html

I dare to point out this: How important is it for you to give each object the chance to perform its last task before deletion?
http://www.cprogramming.com/tutorial/constructor_destructor_ordering.html

Can it be that the value of a corresponding resource release operation is occasionally underestimated?
(Pointers will not be reset to the safe value "nullptr" for example which might result in unwanted effects if affected pointers were passed around in the software for other uses.)

[Erik Ogenvik (erik-ogenvik)]

Yes, that's a different and very valid reason for wanting to do it.
There's absolutely an advantage in making sure that destructors are correctly called.

As I've said from the start we already use uniqe_ptr in many places, that there are places where we probably should use it more, and likewise places where we wouldn't gain anything from using it.

I still don't fully understand what you want with this bug report. If it's "replace all raw pointers with unique_ptr" I don't agree, but if it's "replace raw pointers with unique_ptr where suitable" I'm for it.
But do note that you probably won't find many places where it's suitable. There are very few places where there can be unexpected and recoverable exceptions thrown in constructors.
We do not consider std::bad_alloc to be a recoverable exception, so there's no point in catching it or even try to write defensive code for it. The process will go down and then destructors being called or not doesn't matter.

[Markus Elfring (elfring)]

Thanks that you can agree to a part of my wording adjustment.

I would appreciate a clarification for the disagreement about the suitability of "unique_ptr" in remaining source code places.

[Erik Ogenvik (erik-ogenvik)]

What do you need explained?

Is your position that all raw pointers should be replaced with unique_ptr?

[Markus Elfring (elfring)]

I am looking for the background information which classes belong to sources from other software. I try to avoid to touch source code here which might be maintained by other developers who are not members of your project.

If there would be a clearer separation of components, I could say a bit easier where raw pointers should still be replaced by instances of "unique_ptr" from my point of view.