Hi all,

I think I=92ve found what may be a Mysql++ bug related to the ResUse
destructor. Although my own code where I=92ve encountered the problem =
is
large and complicated, I=92ve boiled it down to a simple testcase as
illustrated below. See comments in code.


//// start
#include <mysql++/mysql++.h>=20
#include <iostream>

// adjust as needed for your server
#define DB_DB "test"
#define DB_HOST "myhost"
#define DB_USER "myuser"
#define DB_PASS "mypass"

using namespace std;
using namespace mysqlpp;

int main(int argc,char **argv) {

// in my code I need to have control over if and when a connection is =
made
// and then want to pass around a pointer to the connection to things =
that

// need it. So I create a connection as follows instead of doing
// mysqlpp::Connection con(false); as in the examples
Connection *con=3Dnew Connection(false);

// connect is a separate operation since there's no way to=20
// disable exceptions with the connect on create method
con->connect(DB_DB,DB_HOST,DB_USER,DB_PASS);

// create a query and get results
Query q=3Dcon->query();
ResUse use=3Dq.use("show variables like 'version'");

// show results
while(Row row=3Duse.fetch_row()) {
cout << row.at(0) << "=3D" << row.at(1) << endl;
}
=20
// close connection
con->close();

// done with it, delete it
delete con;

// now I wouldn=92t expect to do anything
// anymore with row or use now that connection
// has been closed and deleted, however,=20
// 'use' goes out of scope here
// and it's destructor is invisibly called
// problem is, it references an internal pointer
// to con, which no longer is valid (result.cpp, 69)
// a crash may result.
}
//// end=20


Code may or may not crash depending on how things may be arranged, your
compiler, and platform (GCC4 on linux x86 in my case). I first found =
the
bug with valgrind (www.valgrind.org)

It can be worked around by not using a pointer, but it would be =
inconvenient
in my code and nothing in the documentation says I can't or shouldn't do =
it
this way. =20

Even then, if I don't use pointers I'm not sure it isn't still a problem =
as
there's nothing that guarantees in what order C++ calls destructors when
stack allocated variables go out of scope and Connection could be =
deleted
before ResUse.

Thanks.

-----
David Wojtowicz, Sr. Research Programmer, Sysadmin
Dept of Atmospheric Sciences / Computer Services
University of Illinois at Urbana-Champaign
davidw (AT) uiuc (DOT) edu=A0 (217) 333-8390



--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw

This behaviour is by design, although this consequence was perhaps
unintended. Given that a query is based on a connection, and ResUse is
based on a query, the order of destruction must be in the reverse of
construction.

In MySQL terms, it should make sense as well, given that a ResUse only
retrieves data as needed, and so must have a connection involved.

One way to fix your issue is to wrap your data access in a scope of its
own. You can do this with a class or a function.

Taking your example, you can modify it like this:

int main(int argc,char **argv)
{
Connection *con=new Connection(false);
con->connect(DB_DB,DB_HOST,DB_USER,DB_PASS);

{
// create a query and get results
Query q=con->query();
ResUse use=q.use("show variables like 'version'");

// show results
while(Row row=use.fetch_row()) {
cout << row.at(0) << "=" << row.at(1) << endl;
}
}

// close connection
con->close();

delete con;
}

In your application, you can create a proper scope by putting that nested
scope in a function or class of its own.

Alternatively, use C++ mechanisms for this, which is safer anyway when
exceptions are involved:

int main(int argc,char **argv)
{
std::auto_ptr<Connection> con(new Connection(false));
con->connect(DB_DB,DB_HOST,DB_USER,DB_PASS);

// create a query and get results
Query q=con->query();
ResUse use=q.use("show variables like 'version'");

// show results
while(Row row=use.fetch_row()) {
cout << row.at(0) << "=" << row.at(1) << endl;
}
}

- Chris


--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw

Chris,

Thanks for your response. I did understand that things need to be
destructed in the proper order and how I might attempt that (though I hadn't
thought of using std::auto_ptr, which I've never used before...cool)

I guess my main point was that it is too easy to create code that looks
perfectly valid (like my example), but had unintended side effects, due to
the hidden nature of destructors being called as objects go out of scope.

Here is an even simpler piece of code that runs into the same problem.

int main(int argc,char **argv) {

ResUse use;
Connection con(....);
Query q=con.query();
use=q.use("....");

}

I don't know if the C++ standard prescribes the order in which destructors
must be called when multiple objects go out of scope or not, but gcc anyway
reasonably destructs them in the reverse order they were constructed in.

In the above code 'con' gets destructed before 'use', which is a problem.
Granted, if you understand the internals of mysql++, then you can see why
this is a problem. But it's not entirely obvious to anyone else. After
all, you can do without repercussions:

{string A; string B; B="text"; A=B;}

Some possibilities to avoid this pitfall in mysql++:

1) If ResUse::~ResUse() didn't need to call conn_->unlock(), the problem
would be solved. I admit I'm not familiar enough with the internals to know
why it is needed and there probably is a very good reason, though I can't
easily see what it is. I tried commenting it out and recompiling and
running some of my programs with it and didn't run into problems. Nothing
else calls unlock in result.cpp, and there's no guarantee when ResUse will
be destructed, so it doesn't seem to be important for continuing onto the
next transaction. But again, I'm probably missing something.

2) Make the default ctors for ResUse and Result private (which is not
unlike there being no default ctors for Query and Connection) This would
require making Query a friend of both classes. It would prevent you from
compiling the problem code example above... and force you to create a ResUse
from an existing object, thus in the proper order. Though it won't help in
my original example where I explicitly called delete on the Connection.

3) Add a note to the docs, warning about deleting Connection before things
that depend on it go out of scope.


Perhaps it's not a big deal, but I spent like half a day tracking down this
mysterious crash in my code, where it wasn't so obvious due to the number of
different places the various calls are used.

Thanks for listening.


-----Original Message-----
From: Chris Frey [mailto:cdfrey (AT) foursquare (DOT) net]
Sent: Tuesday, November 01, 2005 3:02 PM
To: plusplus (AT) lists (DOT) mysql.com
Subject: Re: mysql++ connection/resuse destructor bug

This behaviour is by design, although this consequence was perhaps
unintended. Given that a query is based on a connection, and ResUse is
based on a query, the order of destruction must be in the reverse of
construction.

In MySQL terms, it should make sense as well, given that a ResUse only
retrieves data as needed, and so must have a connection involved.





--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw

On Tue, Nov 01, 2005 at 07:05:20PM -0600, David Wojtowicz wrote:
I do agree, it is too easy to write code like this, and I'm in favour
of making it hard to use code wrong. Good example too.


Yes, the C++ destruction order guarantees a bug with the above example.


The mysql++ classes are not completely independent like string, so even
if we wanted to, I don't think that goal is appropriate.


I like this idea. It helps to force the correct order of construction.
The only problem I can think of is that it may make it harder to build
a class that contains a ResUse object. It forces the query to happen
in the constructor of such a class, or it forces dynamic allocation.

- Chris


--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw

On Tue, Nov 01, 2005 at 07:05:20PM -0600, David Wojtowicz wrote:
Overall, I believe this unlock() call is just a safety / sanity check.
It should be removed. The idea that a lock()/unlock() spans two classes --
Query and ResUse -- seems wrong to me.

See line 481 in query.cpp. This is where the lock() could happen, and
all unlock() paths appear to be covered.

The only use of the lock mechanism in ResUse is to unlock. This goes against
the idea that lock/unlock combos should be close together in the code, and
always paired.

I've removed it in SVN.

- Chris


--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw

Chris Frey wrote:
No, it's a half-assed attempt at enforcing the MySQL C API's
restrictions on the use of connections:

http://dev.mysql.com/doc/mysql/en/threaded-clients.html

Proper locking is necessary in the face of threads. Right now, you must
handle this explicitly in your own code, but there's no good reason why
MySQL++ can't handle this internally. When we add proper mutex support
in v2.1, the Lockable feature will pull its own weight.

I'm okay with your reasoning to remove the ResUse unlock call. I don't
know for certain why it was there. Perhaps we are missing something,
and will end up with a deadlock when we get start using real mutexes in
multithreaded programs.

--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw

David Wojtowicz wrote:
Some of this is a documentation problem. Up to now, we've been
depending on the existence of the examples to cover this. That is, do
things like the examples, and you'll be fine. These issues should be
explicitly covered in the user manual.

You can get a PDF copy of the standard for only $18 here:
http://webstore.ansi.org/ No excuses now.

I'll be shipping v2.0.7 in the forseeably near future, and it will have
Chris's patch for this in it. I'm not certain that this is a completely
correct fix, but it's certainly part of a correct fix.

Hmmm, maybe. This would have to wait for v3.0.

I had in mind a much more extensive discussion. The dependencies are
more complex than that.

Thanks for complaining constructively.

--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw