Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bad database_binder destructor behavior #63

Closed
polesapart opened this issue Dec 19, 2016 · 2 comments
Closed

Bad database_binder destructor behavior #63

polesapart opened this issue Dec 19, 2016 · 2 comments

Comments

@polesapart
Copy link

polesapart commented Dec 19, 2016

database_binder design suffers from a minor shortcoming. It has a defaulted move constructor and a destructor path that assumes every object was fully initialized by the parametrized constructor.

These are subtle contradictions, because when the move constructor is used for in-place initialization, the "moved" object is empty. So, for instance:

database_binder
    class dbFront {
    std::unique_ptr<database_binder> storedProcedure;
    database db;
     dbFront(const std::string &dbFile, const std::string &key): db(dbFile) {
     db << "PRAGMA key = '" << key << "'";
     createTables(db);
     storedProcedure = std::make_unique<database_binder>(db << "INSERT INTO log (a, b) VALUES(?,?)");
    }

(I understand the unique_ptr implications, it's beside the point. There are other scenarios where move constructor would apply. The reason unique_ptr was chosen is that I need to initialize the stored procedure as class members and there's no default constructor. We could add one, but, if at a later time, we decided to replace/update the stored procedure object [by move], the bug would trigger).

There are "two objects" created on the storedProcedure = ... line; the one returned by db << "..." which is moved into the empty one allocated by std::make_unique.

The default move construtor works ok, the problem with above is that the object returned by the "db << "..." statement, after the move, is replaced by the empty object, and this one has no statement at the _stmt member, nor a pointer to the db for that matter.

The empty object destructor is called on site, and it tries to call execute(), which won't work and throws.

There are many ways to fix it - I think the most polite would be to have a custom move construtor, but what I did instead was to check if the _stmt is valid and only call execute in this case:

~database_binder() noexcept(false) {
			/* Will be executed if no >>op is found, but not if an exception
			is in mid flight */
			if(!execution_started && !std::uncaught_exception() && _stmt) {
				execute();
			}
		}
@aminroosta
Copy link
Collaborator

@polesapart Thanks very much for the detailed explanation.
As soon as i saw "... It has a defaulted move constructor" i knew exactly what's coming :-)

I've implemented #64 which is exactly the solution you described.

@polesapart
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants