Avoid exception from destructor while stack unwinding

[mloskot]

Maciej notified me about discussion on SOCI at Linkedin C++ Professionals Group, where folks discussed that SOCI still suffers from potential of throwing from destructor while another exception is handled.

Maciej suggested to fix it by checking std::uncaught_exception() in ref_counted_statement_base::dec_ref(), and if it returns true, the final_action() should not be called to avoid _any_ interactions with database.

[vadz]

FWIW I totally agree with http://www.gotw.ca/gotw/047.htm and think that using uncaught_exception() is a bad idea and instead we should never throw from this dtor at all. Of course, this means that we need to suppress (i.e. catch and not rethrow) all the exceptions that can happen here and do something with them, probably just remember the error message and set some error flag, which is more difficult than the proposed solution. But it's also much more robust IMO, not being able to rely on RAII in presence of exceptions is like not being to count on your umbrella when it rains.

[mloskot]

@vadz I sympathise with your point, but it would mean changing behaviours that have been there by-design. Let me use Roger's explanation that every SOCI user should know:

The following code:

sql <<  "select name, salary from persons where id = "<<  id,
      into(name), into(salary);

invokes the actual execution of the database query in the destructor of the soci object returned from operator<< at the closing semicolon. Any SQL exception will be thrown from the destructor.

AFAIU, you suggest to redesign it to something like this:

sql <<  "select name, salary from persons where id = "<<  id,
      into(name), into(salary);
if (!sql)
{
    std::error_code ec = sql.error();
    ...
}

(Similar approach is used in https://github.com/chrismanning/ejpp/)

Then, SOCI caughts any exceptions in destructor but never rethrows (like std::basic_filebuf if close() fails).

I think that would be acceptable change in behaviour in SOCI 4.x.

[ArnaudD-FR]

Hi,

I feel having such change in design will break error management in any program using soci. Even if this change is done in a new major version it will be hard and error prone to switch to it.

Is the nothrow(false) annotation not enought to solve this for C++11 compilers?

If really we can not use the nothrow(false) annotation, I don't like your proposal of checking error code in soci::session itself like you suggested:
sql << "select name, salary from persons where id = "<< id,
into(name), into(salary);
if (!sql)
{
std::error_code ec = sql.error();
...
} In my mind checking session itself means is the database link can be used or not.

I would suggest something like close from std::basic_filebuf you refered to but replace it by execute(). If execute is not called manually destructor will call it and ignore any exception .

So the following statement will ignore errors
sql << "select name, salary from persons where id = "<< id,
into(name), into(salary);
Next ones will execute and throw exception
stmt = sql << "select name, salary from persons where id = "<< id,
into(name), into(salary);
stmt.execute(); // throw error

or in short :

(sql << "select name, salary from persons where id = "<< id,
into(name), into(salary)).execute();

This is just a quick idea and I didn't check implication with soci::statement returned from sql.prepare

Arnaud

----- Mail original -----

De: "Mateusz Łoskot" notifications@github.com
À: "SOCI/soci" soci@noreply.github.com
Envoyé: Mercredi 2 Juillet 2014 23:34:21
Objet: Re: [soci] Avoid exception from destructor while stack unwinding (#256)

@vadz I sympathise with your point, but it would mean changing behaviours that have been there by-design. Let me use Roger's explanation that every SOCI user should know:
The following code: sql << "select name, salary from persons where id = "<< id,
into(name), into(salary);
invokes the actual execution of the database query in the destructor of the soci object returned from operator<< at the closing semicolon. Any SQL exception will be thrown from the destructor .
AFAIU, you suggest to redesign it to something like this: sql << "select name, salary from persons where id = "<< id,
into(name), into(salary);
if (!sql)
{
std::error_code ec = sql.error();
...
}
(Similar approach is used in https://github.com/chrismanning/ejpp/ )
Then, SOCI caughts any exceptions in destructor but never rethrows (like std::basic_filebuf if close() fails).
I think that would be acceptable change in behaviour in SOCI 4.x.
—
Reply to this email directly or view it on GitHub .

[lukeocamden]

I feel like I'm missing something. What exactly is the problem here? Is there a test case we can base this discussion on?

[mloskot]

@lukeocamden Good question. The issue is just a forward of message I received from Maciej and that I planned to look further. The original discussion took place on the Linkedin C++ group and is a bit lengthy (if you have access to that group, you may read through it).

Shortly, a general and recommended rule of not throwing destructors was confronted with existing code that allows throwing destructors, with SOCI as an example.
Maciej defended SOCI very well (see below) and there is no issue indeed, but uncaught_exception() was suggested to handle cases of incredibly convoluted usage (which nobody has seen so far, AFAIK).
Then, the addition of uncaught_exception() was objected by @vadz on the grounds of the general rule destructors shall not throw.
That's how we've taken detour and started brainstorming how to make SOCI destructors non-throwing.

👍 for Maciej's suggestion from me.

Here is concluding word from Maciej in that discussion (hope Maciej doesn't mind me paste it):

---

It's about the lifetime of the temporary.

sql << "select x from y", into(x);

The actual interaction with the database happens at the end of the full expression; conceptually and to ease visualization, we can think that the actual work happens exactly where the semicolon is at the end of the line above. All sub-expressions above have the interesting property of forcing the ordering of side effects, so indeed, the only way to reach the semicolon is to successfully execute everything else on the left. In other words, if you fail earlier, there is no point to interact with the database at the end. It is true that all involved objects are on the stack, but it is easy to ensure that the database interaction does not happen during stack unwinding.
The standard uncaught_exception() can be helpful, too, to protect against some incredibly convoluted usage.

There is really no magic here and all this is guaranteed and explained by the standard.

[lukeocamden]

Thanks for the background. I'm trying to imagine what usage could go wrong. Is it possible that people are imagining doing something like this:

auto x = (sql << "select x from y", into(x));
this_may_throw();

It should be doable to prevent copy/move construction of x in this way. People still have sql.prepare if they want delayed execution.

[vadz]

I don't think there is anything especially convoluted here. Suppose you want to store some information about the failure of some operation in the database and so use SOCI while handling some other exception -- currently this would result in an abort.

[lukeocamden]

If a user wants to contact a database in a destructor, they do this:

~MyType()
{
  try
  {
    sql << "select x from y", into(x);
  }
  catch (...)
  {
    // User can put something here
  }
}

[vadz]

This is not what I mean. You could instead be using SOCI while already handling an exception (not necessarily in dtor, although could be as well).

[lukeocamden]

I'm not sure I get it - sorry for not keeping up. Could you illustrate in code?

[ArnaudD-FR]

for example you manage SQL transaction in a RAII object.

struct SqlTransaction
{
    SqlTransaction(soci::session &sql)
    { sql << "BEGIN"; }

    ~SqlTransaction()
    { sql << "ROLLBACK"; }

    ....
};

void my_insert(soci::session &sql)
{
    SqlTransaction transaction(sql);
    int a;
    sql << "insert into mytable(a) values(?)" , use(a);
}

If you lose your database connection right after initialization of transaction an exception will be thrown by

sql << "insert into mytable(a) values(?)" , use(a);

but transaction instance will be destroyed and it will try to rollback session. So a new exception will be thrown by

sql << "ROLLBACK"; 

and your program will be finished

[lukeocamden]

Shouldn't ~SQLTransaction look like this?

~SqlTransaction()
{
  try
  {
    sql << "ROLLBACK";
  }
  catch (...)
  {
    // Do something smart
  }
}

If user code does some database operation in any RAII destructor, they need to prevent exceptions being leaked - this is true even if we don't throw from any SOCI destructor. For example, if the design were changed so that operator<< reports the error by throwing, your program would still crash unless you add a try/catch.

[vadz]

@ArnaudD-FR example does show it, but it could be even more innocent than this: imagine you use a log framework (think of Boost.Log if you want, but it could be any of them) which allows you to define custom loggers. Now suppose you want to log failures of some operations, signalled -- of course -- by exceptions. So your logging code is executed during exception handling.

Now there is nothing wrong with using logging in an exception handler, especially if you take care to put the logging code inside try{...}catch(...). There is also nothing wrong, in principle, with using a database for logging, especially if you take care to catch all the exceptions thrown by database operations. And there is definitely nothing wrong with registering a custom logger using SOCI to write the log message to a database table.

Unfortunately the combination of all these steps is fatally flawed because any error while writing the log message to the database will terminate the program, in spite of all precautions you had taken. And when a combination of reasonable expectations results in an unreasonable behaviour, something is very wrong somewhere. And I'm afraid that in this case, this "somewhere" is SOCI :-(

[lukeocamden]

Similarly, @vadz, shouldn't your custom logging code look like this:

// May be called from a destructor
void log_to_database(...)
{
  try
  {
    sql << ...;
  }
  catch (...)
  {
    ...
  }
}

[ArnaudD-FR]

@lukeocamden this was just a stupid example demonstrating that on some case a second exception may be throw by statement destructor

[lukeocamden]

My point is that the expression "sql << ..." may throw to report an exception, the exact site of the throw statement is not relevant. If you try to access a DB from a destructor then you have to be prepared for the DB code to throw - this is the responsibility of the user. In the logging case, either

1. The logging API should guarantee it does not throw, in which case it should handle the exception

or

2. The logging API may allow it to throw, in which case it is down to the user.

[ArnaudD-FR]

I think we are going away from main subject, listed example might exist, code is never perfect and some complex use cases where 2 exceptions are thrown and not catch might exist.

Returning to the subject, as I understand c++11 does not allow destructors to throw exception unless specified. So @mloskot proposed to do a brainstorming to update or not soci to match c++11 requirements.

You can read my 2 cents on my first post

[lukeocamden]

OK - going back to your first post. You say "Is the nothrow(false) annotation not enought to solve this for C++11 compilers?". My answer is a big yes, ie what is already implemented is enough. If a user wishes to access a DB from a destructor, they have the power to decide whether to allow exceptions to escape (and risk std::terminate), or use try/catch to do something more resilient.

[pfedor]

But this has nothing to do with whether SOCI throws from a destructor.
You'd have the same problem if it throwed from the constructor or
operator<<. The only example where it is relevant that SOCI throws from a
destructor is the one lukecamden gave, with

auto x = (sql << "select x from y", into(x));
this_may_throw();

which indeed is convoluted usage.

Thanks,

Aleksander

On Thu, Jul 3, 2014 at 5:23 AM, ArnaudD-FR notifications@github.com wrote:

for example you manage SQL transaction in a RAII object.

struct SqlTransaction
{
SqlTransaction(soci::session &sql)
{ sql << "BEGIN"; }

~SqlTransaction()
{ sql << "ROLLBACK"; }

....

};

void my_insert(soci::session &sql)
{
SqlTransaction transaction(sql);
int a;
sql << "insert into mytable(a) values(?)" , use(a);
}

If you lose your database connection right after initialization of
transaction an exception will be thrown by

sql << "insert into mytable(a) values(?)" , use(a);

but transaction instance will be destroyed and it will try to rollback
session. So a new exception will be thrown by

sql << "ROLLBACK";

and your program will be finished

—
Reply to this email directly or view it on GitHub
#256 (comment).

[pfedor]

On Thu, Jul 3, 2014 at 5:32 AM, VZ notifications@github.com wrote:

@ArnaudD-FR https://github.com/ArnaudD-FR example does show it, but it
could be even more innocent than this: imagine you use a log framework
(think of Boost.Log if you want, but it could be any of them) which
allows you to define custom loggers. Now suppose you want to log failures
of some operations, signalled -- of course -- by exceptions. So your
logging code is executed during exception handling.

Now there is nothing wrong with using logging in an exception handler,
especially if you take care to put the logging code inside
try{...}catch(...). There is also nothing wrong, in principle, with using
a database for logging, especially if you take care to catch all the
exceptions thrown by database operations. And there is definitely nothing
wrong with registering a custom logger using SOCI to write the log message
to a database table.

Unfortunately the combination of all these steps is fatally flawed because
any error while writing the log message to the database will terminate the
program, in spite of all precautions you had taken.

If you put the logging code inside try{...}catch(...), then why would it
terminate the program?

Thanks,

Aleksander

And when a combination of reasonable expectations results in an
unreasonable behaviour, something is very wrong somewhere. And I'm afraid
that in this case, this "somewhere" is SOCI :-(

—
Reply to this email directly or view it on GitHub
#256 (comment).

[mloskot]

@ArnaudD-FR

Returning to the subject, as I understand c++11 does not allow destructors to throw exception unless specified. So @mloskot proposed to do a brainstorming to update or not soci to match c++11 requirements.

This aspect of coping with changes between C++03 C++11, as noted by Roger Orr on the mailing list long time ago, has already been taken care of, see #181 & #192

#192

[mloskot]

On 3 July 2014 20:13, Pawel Aleksander Fedorynski notifications@github.com
wrote:

But this has nothing to do with whether SOCI throws from a destructor.
You'd have the same problem if it throwed from the constructor or
operator<<. The only example where it is relevant that SOCI throws from a
destructor is the one lukecamden gave, with

auto x = (sql << "select x from y", into(x));
this_may_throw();

which indeed is convoluted usage.

@pfedor, thanks for chiming in and I agree with you.

@lukeocamden gave an interesting example, but i doubt it's expected to be used in practice.
We can always document it and discourage such convoluted use of the library objects.

General recommendations like those given by Sutter are good, but they are...general and in our case they smell of a dogma that does not necessarily fit the reality.

We should align to technical specification of C++ standard only and according to that, in SOCI, we throw from destructors by design and safely.

Finally, we can't simply change the status quo here, unless we aim a different SOCI, really.

Addition of the proposed std::uncaught_exception check could be equally turned into assert(!std::uncaught_exception()), so it's purpose sticks out.
We can have both, if and assert, though.

I'm in favour of merging the proposed #257.

[vadz]

The problem with #257 is that if you have code like this:

Foo::~Foo()
{
    try {
        // Suppose that Log() ends up using SOCI to write the message to
        // the database.
        Log("Foo being destroyed");
    } catch (...) {
        // whatever     
    }
}

and Foo is being destroyed as part of stack unwinding, we won't even attempt
storing the log message in the database because uncaught_exception() returns
true at the moment when Log() is called.

Now, this is probably not a fatal problem, but it's still definitely unexpected and rather nasty because the bug is going to appear "randomly".

Whether it's better than working normally most of the time (in this situation) and aborting the program if a database write error occurs is not really clear.

Perhaps we should test for uncaught_exception() before rethrowing the exception, but still try to perform the final_action() in any case?

[lukeocamden]

Swallowing the exception is still wrong IMO. Presumably the user wrote code to contact the DB for a reason. Failing to check for errors (by try/catch) is a bug and the runtime is totally justified in calling std::terminate in this case. This could imaginably be much better than continuing as if nothing is wrong - it could be the difference between some vital diagnostic information being retained in a core file, or lost without a trace.

[pfedor]

Yes, exactly. Using uncaught_exception() would break this perfectly valid
usage, in order to enable extremely convoluted usage which we all agree is
never expected to be seen in practice.

I think we should keep things exactly as they are.

Cheers,

Aleksander

On Fri, Jul 4, 2014 at 8:35 AM, VZ notifications@github.com wrote:

The problem with #257 #257 is that if
you have code like this:

Foo::~Foo(){
try {
// Suppose that Log() ends up using SOCI to write the message to
// the database.
Log("Foo being destroyed");
} catch (...) {
// whatever
}}

and Foo is being destroyed as part of stack unwinding, we won't even
attempt
storing the log message in the database because uncaught_exception()
returns
true at the moment when Log() is called.

Now, this is probably not a fatal problem, but it's still definitely
unexpected and rather nasty because the bug is going to appear "randomly".

Whether it's better than working normally most of the time (in this
situation) and aborting the program if a database write error occurs is not
really clear.

Perhaps we should test for uncaught_exception() before rethrowing the
exception, but still try to perform the final_action() in any case?

—
Reply to this email directly or view it on GitHub
#256 (comment).

[mloskot]

Given the subtle differences in opinions the proposed changes in SOCI run-time behaviour, I agree to keep things exactly as they are.

What about adding assertion? Not necessary, not recommended, neutral?
My point is that while debugging user would get better context, before crash happens,
and those who read SOCI sources would have chance to learn how to use it better.

[lukeocamden]

Asserting on std::uncaught_exception is wrong because the following code would then stop working:

SomeObject o;
throw 5;
...
SomeObject::~SomeObject()
{
  try
  {
    // NB std::uncaught_exception is *true* here, but the code is not wrong
    sql << ...;
  }
  catch (...) { }
}

[ArnaudD-FR]

I agree with @lukeocamden, you can not do it in a neutral way

[mloskot]

I should have referred to @vadz suggestion:

Perhaps we should test for uncaught_exception() before rethrowing the exception, but still try to perform the final_action() in any case?

IOW, assumption like here:

try
{
    final_action();
}
catch (...)
{
   // assume no uncaught exception here
   delete this;
   throw;
}

[lukeocamden]

Where you write 'assume no uncaught exception here', std::uncaught_exception may return true if you code is called from a destructor.

[ArnaudD-FR]

even if you do something like

bool backupFlag = std::uncaught_exception();
try
{
    final_action();
}
catch (...)
{
   // assume no uncaught exception here
   assert(backupFlag);
   delete this;
   throw;
}

You will break example given by @lukeocamden because exception was exepected to be caught at higher level

[mloskot]

@lukeocamden That is the point. The assertion will be followed with program termination anyway.

@ArnaudD-FR What you mean as "break example"? I simply suggest to document a crash case (assertion is a documentation, and a check active in debug-only build).

Nevertheless, there is nothing to break as we talk about program termination. Moreover,
SOCI use as in @lukeocamden example will likely cause stack overflow due to competing throw and delete this statements, so sql::~sql never exits.

struct sql
{
    ~sql()
    {
        try
        {
            throw 0;
        }
        catch (...)
        {
            //SOCI_ASSUME(!std::uncaught_exception());
            delete this;
            throw;
        }
    }
};

try
{
    sql s;
    throw 1;
}
catch (...) {}

[lukeocamden]

My example won't terminate if the user has written try/catch in whatever destructor calls into SOCI. For example:

// May be called during stack unwinding, in which case std::uncaught_exception will return true
~MyObject()
{
  try
  {
    // SOCI must not abort here. Any exception thrown will be caught by the following catch
    sql << ...;
  }
  catch (...)
  {
    // This will be run, terminate will not be called
  }
}

[mloskot]

The try/catch above does not really matter, as the execution will get stuck in the sql << ...; line, until stack is overflown. I'm quite convinced that's what will happen, unless I don't see complete picture of your case.

Are we arguing about equivalent situation to this?

struct sql
{
    ~sql()
    {
        try
        {
            // final_action throws
            throw 0;
        }
        catch (...)
        {
            delete this;
            throw;
        }
    }
};

struct client
{
    ~client()
    {
        try
        {
            sql s;
            throw 1;
        }
        catch (...) {}
    }
};

int main()
{
    try
    {
        client c;
        throw 2;
    }
    catch (...) {}

    return 0;
}

[lukeocamden]

'delete this' in a destructor is no good, but AFAICT SOCI does not do that. I think I've missed something.

[mloskot]

I've updated the code above to replicate SOCI exceptions route.
I hope I have managed to capture the situation, haven't I?
I believe it does lead to std::terminate anyway. Unless, I'm missing something this time, am I?

struct ref_counted_statement_base
{
    virtual ~ref_counted_statement_base() {}
    virtual void final_action() = 0;
    void dec_ref()
    {
        try
        {
            final_action();
        }
        catch (...)
        {
            delete this;
            throw 2;
        }
    }
};

struct ref_counted_statement : ref_counted_statement_base
{
    void final_action()
    {
        try
        {
            throw 1;
        }
        catch (...)
        {
            throw;
        }
    }
};

struct sql
{
    sql() : rcst(new ref_counted_statement()) {}

    ~sql() 
    {
        rcst->dec_ref();
    }

    ref_counted_statement* rcst;
};

struct client
{
    ~client()
    {
        try
        {
            sql s;
            throw 0;
        }
        catch (...) {}
    }
};

int main()
{
    try
    {
        client c;
        throw 2;
    }
    catch (...) {}

    return 0;
}

[lukeocamden]

Your code is missing 'noexcept(false)' on ~sql()

Other than that your code:

sql s;
throw 0;

is the same as my earlier example:

auto x = (sql << ...);
throw 0;

In real usage the object whose destructor throws does not live past the end of the statement, because people write it like this:

session << "select foo from bar";
this_may_throw(); // No issue here, no ref-counted-statement to destroy

[mloskot]

On 8 July 2014 16:56, lukeocamden notifications@github.com wrote:

Your code is missing 'noexcept(false)' on ~sql()

Yes, the master still needs to build in C++03.

Other than that your code:

sql s;
throw 0;

is the same as my earlier example:

auto x = (sql << ...);
throw 0;

Yes.

In real usage the object whose destructor throws does not live past the
end of the statement, because people write it like this:

session << "select foo from bar";
this_may_throw(); // No issue here, no ref-counted-statement to destroy

Yes, I've made a round about the convoluted case indeed.
It helped me to confirm myself some of the subtle details.
At least, I've convinced myself there is no clean way to help users with
assertion as a tool to detect
unsupported situations like you described: "that failing to check for
errors (by try/catch) is a bug".
Any such attempt would be too strict preventing some of correct uses, what
@ArnaudD-FR also mentioned.

I believe we all agree to keep things as they are and close this Issue/PR
without merging.

[lukeocamden]

I definitely agree.