Hi Ross,
On Thu, Aug 27, 2009 at 10:08:31PM -0700, Ross McFarland wrote:
> the way ldrizzle-proto's connection works is that you do
> drizzle_connection_initialize(&con, &drizzle, dbname);
> drizzle_connection_connect(&con)
> drizzle_connection_set_tcp(&con, host, port);
> drizzle_connection_set_auth(&con, user, pass);
> set_tcp and set_uds are orthogonal, set_auth could apply to either.
Yeah, same in libdrizzle.
> you already have these functions in conn.h, but there's api on drizzle (named
> drizzle_con_*) that supplants it in a limited fashion. it's much cleaner to
> simply create a connection with drizzle_con_create (with an added db param)
> and
> then call set_tcp and/or set_auth on it if necessary.
So, these all work just fine in libdrizzle as well, except that dbname
should actually be optional as well. You may want to connect to the
database without a specific database context (ie, if no databases
exist yet and you need to "create database ..." first).
> there's 8 ways to do one
> thing and the one you're seeming to prefer (drizzle_con_add_tcp) is the least
> obvious. it's named like it's a method on connection, but it really adds one
> to
> a drizzle object....
Understood on the naming, and for that matter we may actually want
to move drizzle_con_create() to the "drizzle" object as well (since
you are really working on a "drizzle" object). Perhaps have:
/* Allocate space and init a new connection object. */
drizzle_create_con(drizzle);
/* Same, but convenient for tcp/uds. */
drizzle_create_con_tcp(drizzle, host, port, ...
drizzle_create_con_uds(drizzle, socket, ...
/* Init a pre-allocated connection object. */
drizzle_init_con(drizzle, con);
/* Same, but convenient for tcp/uds. */
drizzle_init_con_tcp(drizzle, con, host, port, ...);
drizzle_init_con_uds(drizzle, con, socket, ...);
This is now in the context of a drizzle object, since you are always
adding a connection to a drizzle object. The _tcp/_uds versions are
just handy to reduce the number of parameter calls for many cases.
> > A connection is *always* created in the context of drizzle object,
> > so you are never required to free the connection object manually
> > (this will always get done in the drizzle object destroy). You can
> > optionally free a connection resources before a drizzle resource if
> > don't need it anymore.
>
> but you you don't supply a drizzle object one will be created for you, in that
> case you have to destroy the connection object, since you don't have a drizzle
> object.
Yeah, this is where the con_create() function will create a temporary
drizzle object for you. It's still there, just hidden for those users
who only need a single connection. I felt a bit dirty adding this,
but could be convenient for a certain set of users. Perhaps I should
just remove it to avoid confusion. :)
> > The recursive free behavior is there to make the API easier to use,
> > you don't need to track all your objects since drizzle does this
> > for you. For example, you may create a drizzle object, add a couple
> > connections to it, and start running queries against that pool of
> > connections. All you care to manage is the queries and their results
> > after initial setup, it's easier to let the connection pool be freed
> > for you. This also makes it much easier for language wrapper authors
> > to track and free memory (for example, the PHP extension).
>
> but you do end up having to track all of your objects, as you mention in the
> PHP
> bindings case. so every language binding (and non-trivial app) will have to
> keep
> track of exactly how's it using drizzle objects to know if/when they need to
> free what.
It needs to keep state of these objects for some period of time anyways
to complete queries/results/... For example, you may add 3 connections
and 5 queries, and jsut say "run and tell me when it's done". You don't
want to track all that in the meantime, you can just ask for query
results when they complete. Now, say you hit an error and not all
queries complete, you would then need to ask for all the unfinished
objects back and free them manually. To me, it seems a bit easier to
just allow libdrizzle to de-initialize these when need for you.
I understand how this appears to be a bit more of a pain with stack
allocated objects since the function may return and the drizzle
object has a reference to it still. If the drizzle object did NOT
track these and try to clean them up, you still would need to call
destroy() on the object in case it held some other resources (memory,
fds, ...). So, either way you need to destroy() your stack-allocated
references before they go out of scope.
> the part that's annoying is that libdrizzle is keeping track of half
> of that information already so that it can help the client out, but not enough
> information to actually be helpful.
What other information would be useful to track? Every object has
a (void *) member that is for the application to use however it
likes. This allows client-api wrappers to bind their own objects to the
C objects as the need, since these may not always be in scope. There
are even user-defined callbacks to allow you to properly destroy the
(void *) members when things are being cleaned up. I actually spent
a lot of time thinking this part through and also had feedback from
Monty Taylor who did a bunch of other language bindings. I'm very
interested to hear what else may be useful, or some other way to help
track objects for higher level languages.
> at this point it's obvious you disagree, but
> this isn't helpful for non-trivial clients it actually makes it more of a
> pain.
> for instance the bindings will have to keep track of whether a connection
> object
> was created standalone (and thus has a drizzle object created for it inside of
> it) or if it was created with a drizzle object and thus shouldn't free itself.
Yeah, like I said above, I'm just going to remove this and force the
top-level drizzle object always. :)
> trivial modifications of simple.c quickly run in to all sorts of problems with
> this stuff, i just tried a couple things and ran in to problems.
>
> one of them: reusing a result object
> - seems like a perfectly normal/legal/reasonable thing to do
> - if you call drizzle_result_free between the two queries everything
> works fine
> - if you don't call drizzle_result_free things blow up with 10 or so invalid
> frees when drizzle_free is called or drizzle_con_free if you're not
> creating a drizzle object manually.
> - the way things are set up to work if you're using the result object once
> you
> don't have to call drizzle_result_free on it since
> drizzle_free/drizzle_con_free will get rid of it for you, but if you add a
> new query to that function or otherwise reuse the result object all of the
> sudden you do need to be calling free on it.
> - what's worse is the seg-fault happens on cleanup, not where you actually
> did
> something 'wrong' so in a system with 1000's of lines of code you have to
> go
> and dig around and find the place where you happened to reuse a result
> object twice without calling drizzle_result_free in between.
As you saw, the re-use of result objects are fine to use as long as
you _free between each usage. I don't this this is an unreasonable
requirement since you want the user to control the lifetime of these
objects (they may keep one around for a while). The reason why things
blow up is because the result object is re-initialized as if it were
free memory, this means getting put onto the internal list twice.
You are required to free this because if not, you would have a
potential corrupt check on if the result object is initialized. For
example, if you do:
query(con, &result, ...);
/* use result */
query(con, &result, ...);
If query() checks to see if result is valid and needs to be cleaned up
first, you would have an invalid read on un-initialized memory with
the first query() call. On the second query() call, you don't know
if there is just garbage in result from the memory you happen to use,
or an actual result object that needs to be re-initialized.
Not sure how to get around having to put in a _free/_reset result
call in between those.
> > I dealt with this exact case in the PHP extension. You need to do
> > proper reference counting and dependency checking between your objects
> > in these higher level languages to prevent garbage collection on them
> > prematurely. This allows the C resource to exist while the higher
> > level language resources may go away. This is required because you
> > may need to re-create a higher level language resource on an existing
> > underlying resource (for example, con_ready() will return a previously
> > created connection that is now ready again).
>
> my only response is that it doesn't make much sense to me to put the onus on
> every language binding to have to ref-count libdrizzle objects and to have to
> keep up with things like whether or not a connection was created with a
> drizzle
> object or had one created for it, inside of it...
Yup, again regretting that one... :)
As far as refcounting, you can't get around it if you are dealing
with temporary objects in a garbage-collected environment.
The case that causes this is:
function add_queries(queries)
{
drizzle= new Drizzle();
foreach query
con= drizzle->add_con(...)
query= drizzle->add_query(con, query) /* Run query on connection */
return drizzle
}
In a garbage collected environment, the "con" and "query" objects
would be removed after each for loop iteration. The C objects are still
valid underneath though, just contained in the 'drizzle' object. This
is where the references and automatic memory allocation are really
handy in libdrizzle, the language bindings do not need to track con,
query, ..., only the drizzle objects. If the script exits and the
drizzle object goes away, all the cons/queries go with it.
The expense is that if you also have a connection object around, you
need to keep a reference in it to the drizzle object it belongs to
(to make sure the drizzle object does't go away underneath of you).
Any thoughts on how to better handle situations like this?
> i spent a long time looking very closely at/living with gtk+-2.0's api when
> doing gtk2-perl (perl bindings for it.) it has it's warts, but all of the
> (types
> of) things i'm raising here just aren't issues by the nature of the design of
> the api. api design is an art and gtk+ more times than not gets it right. in
> reality the only thing that matters about libdrizzle right now is the api, b/c
> that's what people will be stuck with for the next 10 years. the
> implementation
> can be horrible, but so long as the api is clean/right you can fix that
> transparently over time. if the api (used in a broader sense as the functions
> and defined behaviors) has problems you're stuck with them as soon as there's
> a
> decent number of clients relying on that behavior.
Yup, completely agreed, and is why I've spent more time on the API
and have not done too much on the optimising side yet.
> i'll re-ask the question, in what case would someone actually need it to
> block.
> why not make the default-case be
>
> drizzle_query_str(); // non-blocking, doesn't take a result object
> drizzle_result_read(result); // do this when you want to start getting results
Most folks actually use the behavior of waiting for the result right
away, it's really only more advanced apps where people handle async
stuff properly. Ideally people will start using the async to improve
their apps, which may be enough to make it the default. I'm certainly
up for changing this behavior, this is on the list to the mailing
list of what is preferred.
(Just as a side note, this is actually a blocking write/blocking read
as two separate functions, not really non-blocking. The main point
is that the query() functions never attempt a read() on a result and
instead just return).
> that actually is the behavior i wanted in the first place but i had no idea
> that
> i needed to look at the methods on a result object and options for a
> connection
> to get it. it's another case where there's 4 different paths to get at things
> all of which have non-obvious behaviors.
Agreed, without a query object the connection object was the next
logical place. If we always have a query object than this will be
more appropriate to set there.
> a bit verbose? i started with/ported simple.c and just added stuff as i needed
> to test it out. have a look at simple_port.c i just added, it's 1-for-1:
> http://bazaar.launchpad.net/~rwmcfa1/%2Bjunk/ldrizzle-proto/annotate/head%3A/simple_port.c
> it's ~10 lines longer and deals with fields inside of rows, something that
> libdrizzle will have to do as soon as everything stops being char *.
The "verbose" part I'm talking about is mostly due to the special
case for the first field/row. You init the row, then print the fields
for that row, and then all other rows has it's own field loop as
well. Sure, this can be better solved with a couple functions (like
you did), but this did not seem as intuitive to me.
> while the naming stuff can be chalked up to style/habit, to a certain degree
> it
> matters. consistency is really important, it allows clients to use less brain
> power to remember stuff about your api. using UpCase for something and
> under_scores for others allow you to glance at code and differentiate without
> reading, ... granted things like this are not going to make a functional
> difference in the api, but they'll make it a lot easier to live with.
Agreed, and I've tried to be as consistent in the naming as I could
be. I know there are a few OO-specific issues with first arguments
like we talked about, but for the most part I think it's pretty
clear. Please send me anything you find along the way that looks off
(well, besides anything we've already mentioned in this thread).
> i guess i feel the issues are a little more systemic and can't really be
> addressed (reasonably) in a piece by piece fashion. that and there's obviously
> no consensus between us on what they are.
For most things we've discussed I think we're only a few small API
additions/changes away to being on the same page. Also changing some of
the default behaviors of functions. The only difficult issue we really
diverge on is the recursive memory free()ing of objects. A number of
language APIs have already been designed on this behavior, so it will
be a bit more difficult to change, but if there is a more elegant
way of doing this I'm all for putting the work into doing it that way.
> as i said before there's a limit to
> how much time & effort i want to put in to stating my case (working on fixes
> would be another story) and to be honest i'm getting close to it it.
Understood, these emails take a while to write. :)
> that said, i'm happy to continue to discuss specific cases etc.
Excellent, thank you! I'm probably going to start doing some cleanup
work in a libdrizzle branch to better OO-ify things and change some
of these default behaviors. I'll send you links to the branches when
there is something to check out. I want to get the API nailed down
correctly just as much as anyone (if not more), and as I said, this
feedback is invaluable.
Thanks again!
-Eric
_______________________________________________
Mailing list: https://launchpad.net/~drizzle-discuss
Post to : [email protected]
Unsubscribe : https://launchpad.net/~drizzle-discuss
More help : https://help.launchpad.net/ListHelp
