Exceptions about sqlite_modern_cpp HOT 19 CLOSED

hi @Killili .
i don't understand this part ... if its handled properly it can be ignored.
can you write a piece of code explaining what you have in mind ?

i have something different in my mind.
we must never throw an exception from a destructor

    ~database_binder() {
        /* Will be executed if no >>op is found */
        if (_stmt) {
            int hresult;
            while ((hresult = sqlite3_step(_stmt)) == SQLITE_ROW) { }

            if (hresult != SQLITE_DONE) {
                throw std::runtime_error(sqlite3_errmsg(_db));
            }

            if (sqlite3_finalize(_stmt) != SQLITE_OK) {
                throw std::runtime_error(sqlite3_errmsg(_db));
            }

            _stmt = nullptr;
        }
    }

##here is why.

The C++ rule is that you must never throw an exception from a destructor  
 that is being called during the “stack unwinding” process of another exception.  
For example, if someone says throw Foo(), the stack will be unwound  
so all the stack frames between the throw Foo() and the } catch (Foo e) {  
will get popped. This is called stack unwinding.

During stack unwinding, all the local objects in all those stack frames are destructed.  
If one of those destructors throws an exception (say it throws a Bar object),  
the C++ runtime system is in a no-win situation: should it ignore the Bar and  
end up in the } catch (Foo e) { where it was originally headed?  
Should it ignore the Foo and look for a } catch (Bar e) { handler?  
There is no good answer — either choice loses information.

can you think of a situation that we throw an exception during stack unwinding ?
i don't feel positive about how exceptions are used in this case ...

from sqlite_modern_cpp.

As destructors are commonly called during stack unwinding the exceptions in ~database_binder() mark a situation where we throw during stack unwinding. This is definitly an issue we should fix.

On exceptions in general: I think that exeptions should only be thrown when there is actually exceptional stuff going on. This doesn't include e.g. functions not being called or queries not returing results, in my opinion. As we use the stream operators to provide an interface to the library we could also take the standard library as a model - i.e. it's iostream classes do not throw exceptions by default.

from sqlite_modern_cpp.

Hi,
sry totally forgot about this, you're definitely right. Somehow my java poisoned mind likes constructs like this simple ORM example. That is what i have have in my 'flavored' version of the lib. And i take great care to not throw stuff out of the lambda, but its not obvious that one has to. So i guess that's not a good idea and its not very c++'y.

class ORM {
    std::vector<ORM> childs;
    ORM(int id){
        try {
            db << "select id,data from objects where id = ?" << this->id >> [&](int id, double data){
                this->data = data;
            };
        } catch( NoRowsProcessed& ex ){ // this is an error
            throw ObjectNotfound(); 
        }
        try {
            db << "select id from objects where parent_id = ?" << this->id >> [&](int id, double data){
                this->childs.push_back(ORM(id));
            };
        } catch( NoRowsProcessed& ex ) {} // not an error because not every obj has childs
    }
};

Just for reference why this is a bad thing:

try {
    db << "select id from objects where parent_id = ?" << this->id >> [&](int id){
        this->childs.push_back(ORM(id)); // this might trow a std::bad_alloc and then NoRowsProcessed on unwinding
    };
} catch( exception& ex ) {} // what exception should be handled now?

from sqlite_modern_cpp.

After some thought what about a different operator for throwing exceptions. Like operator>> doing the c++'y way of only throwing real errors (sql exceptions, db issues ...) and operator+ throwing exceptions on logical errors (no lines to process).
Or even a more regexp way where we replace operator>> with operator* for "optional" processing and operator+ for "mandatory" processing.

As in
db << "select * from test" * [&](int val){ }; // might execute lambda
db << "select * from test" + [&](int val){ }; // has to execute lambda or throws
db << "select * from test" >> val; // has to extract single value or throws

i think that would be a consistent approach.

from sqlite_modern_cpp.

using * and + operators will do the job ( and i think introducing new operators is the way to extend the framework).
but at the same time , i also think that the difference between the new operators should be much more than just throwing or not throwing exceptions.
( passing a simple flag to the constructor of database ... ? ).

I have two other scenarios/solutions in my mind.
I will write a pseudocode code to explain them :

     enum sqlite_command {
           not_throw_at_all;
           throw_if_lambda_not_executed;
           throw_??? ;
     }

     db <<  not_throw_at_all << "select * from test"  >> [&](int val) { /* do something */ }

and another idea (which i borrowed from nodejs).

     enum sqlite_errorcode {
            ok;
            not_all_rows_read;
            query_invalid;
            ???;
    }
    db << "select * from test" >> [&](sqlite_errorcode e, int val) { if (e != ok) {  }  }

    // or just something simpler like this
    db << "select * from test" >> [&](bool error_happend, int val) {
             if(error_happend)
                     var msg = db.last_error(); 
    }
   db << "insert int test(a,b) values( ?, ?);" << "A" << "B" >> [&](bool error) { if(error) { /* */ } };

just some ideas but if you think introducing * and + operators are a better way to go, i'm in !

from sqlite_modern_cpp.

I like the expressiveness of the first one, but then if we wanted expressiveness we would not have gone for the intuitive stream operators ;) .
I also like how we could alter behavior on the go e.g.:
db << "sql" << value1 << null_if_nullptr << value2 << value3 << throw_if_norows >> lambda;
Would add a lot of potential but i dont know if its realy needed.

As for the second:
If we would go that way i think most of the errors would lead to a throw anyway because its the only sensible way to get it out of the lambda an there is where the processing would take place. e.g. on the ORM example:

try {
   db << "select id,data from objects where id = ?" << this->id 
         >> [&](sqlite_errorcode e,int id, double data){
                if( e != ok ) throw NoRowsProcessed;
                this->data = data;
            };
} catch( NoRowsProcessed& ex ){ // this is an error
   throw ObjectNotfound(); 
}

And the error would be checked on every line even if it can only happen on the first.

So in conclusion: if we don't want the more expressive way i would start on the intuitive/regexp way of handling it.

from sqlite_modern_cpp.

just one more question (i my mind).
what if we call the given lambda in a try { callback(); } catch(exception e) { } .
and only throw exceptions if the callback didn't throw anything ?

from sqlite_modern_cpp.

I dont get it, call it with what arguments then in case of no rows for example?

from sqlite_modern_cpp.

i will get back to this with an example ...
just a 2 or 3 hours .

@Killili sorry

from sqlite_modern_cpp.

ok.

the problem is that the given lamda must never throw an exception (right ?).

we can call the given lambda within a try {} catch {} block.
something like this :
from line 145 to 162 of the library

template <typename Function>
    typename std::enable_if<!is_sqlite_value<Function>::value, void>::type operator>>(
        Function func) {
        typedef utility::function_traits<Function> traits;

        this->_extract([&func, this]() {
                        try {
                      binder<traits::arity>::run(*this, func);
                        } catch( ...) /*everything*/ {
                              throw_exceptions = false;
                              throw; /* the original exception */
                        }
        });
    }

also take a look at throw inside a catch ellipsis rethrow the original exception

that way the destructor of the of database_binder wont throw an exception and we will not have an stack unwinding problem .

in all other scenarios we will safely throw an exception.

what do you think ?

from sqlite_modern_cpp.

looks like a solution for a different problem, as of now the destructor never throws an exception and we should keep it that way. The destructor sure looks ugly thou. Anyway this would not solve the exception problem if the exception is thrown on the way to the lambda (e.g. operator<< or get_col_from_db) so not a real solution either.

On the other hand i made a quick test of the "options in the stream" thing you mentioned, it will not solve the exception stuff (does it need fixing?). But its a nice to have for the type expansion i added.
I think i like it even more then the new operator stuff. Its not nice to write now because of all the namespace stuff but i will figure out a better way.

example:

int a,b;
db << "insert into test(val,val2) values (?,?)" << a << sqlite::flag(sqlite::options::throw_on_error) << b;

With an extension:

json_spirit::mArray a;
json_spirit::mArray b;
db << "insert into test(val,val2) values (?,?)" << a << sqlite::SpiritOption(sqlite::SpiritOptions::do_something_different) << b;

a will be standard serialized and b will have something different done to it.

from sqlite_modern_cpp.

A version thats more refined is in my fork if you want to have a look.

from sqlite_modern_cpp.

the destructor never throws an exception and we should keep it that way.
why ?

~database_binder() {
        throw_exceptions = false;
        /* Will be executed if no >>op is found */
        if (_stmt) {
            int hresult;
            while ((hresult = sqlite3_step(_stmt)) == SQLITE_ROW) { }
            if (hresult != SQLITE_DONE) {
                throw_sqlite_error(); /* i really want to get this error */
            }
            if (sqlite3_finalize(_stmt) != SQLITE_OK) {
                throw_sqlite_error(); /* and i think this error is (really) important too */
            }
            _stmt = nullptr;
        }
    }

if we know (for sure) that stack unwinding will not happen,
then it's safe to throw exceptions from destructor.
(otherwise some really important exceptions will never be thrown.).

and you are right, it is a solution for a different problem :)

but really what is wrong currently with exceptions ( if there is no stack unwinding ?)

the exception stuff (does it need fixing?)
if all we want is to disable exceptions. then a flag in the constructor will do the job.
or (if you think stream of options is better solution) some command
(sqlite::options::throw_on_error , sqlite::options::throw_on_no_row_returned , ...)
is a solution.

I might be a little confused here ! am i missing something ?

from sqlite_modern_cpp.

thanks i will see the changes in your fork.

from sqlite_modern_cpp.

It will not throw because of the first line!? throw_exceptions = false;

But ok i see the need for the exceptions now, the problem is if we throw the first exception we have a memory leak because the statement will never finalize for example.
What we need is scooped finalizer ... at least i think so i will try to get that in there.

from sqlite_modern_cpp.

I'm asking and we should keep it that way part :)
why we should keep it from throwing exceptions.
(the user might be interested to sqlite3_finilize and sqlite3_step not returning SQLITE_DONE ).

Nice bug hunt for the memory leak, i didn't see that .

from sqlite_modern_cpp.

Ok, update in my fork the destructor now cleans up correctly. As for not throwing stuff out of it we are out of luck, because if we want our lib to to things this way we will have to, so we better make sure its in a friendly way.

I disable all exceptions whenever one is thrown so it "should not" happen that two in a row are thrown from us. Further i cleanup with a scoped lambda that will even after a throw try to cleanup our resources (e.g. the statment) and not throw again.

from sqlite_modern_cpp.

Great work, thanks.

from sqlite_modern_cpp.

Youre welcome, I will give it a try for a few days before i pull it here there might be monster lurking in it.
And of course i need to document it ... somehow.

from sqlite_modern_cpp.