On Thu, Aug 27, 2009 at 3:20 PM, Eric Day<[email protected]> wrote:
> On Wed, Aug 26, 2009 at 09:44:52PM -0700, Ross McFarland wrote:
>> > It's not uncommon for libraries to do the memory management for
>> > you, and like I said, I wanted to make it easy for both types of
>> > users.
>>
>> perhaps a better approach would be to do:
>>
>> drizzle_con_st *drizzle_con_new (drizzle_return_t * ret, <params>);
>> drizzle_return_t drizzle_con_initialize (drizzle_con_st * con, <params>);
>>
>> and drizzle_con_new would just be:
>>
>> drizzle_con_st *drizzle_con_new (drizzle_return_t * ret, <params>)
>> {
>> drizzle_con_st * con = malloc (sizeof (drizzle_con_st));
>> // error handle
>>
>> *ret = drizzle_con_initialize (con, <params>);
>>
>> return con;
>> }
>>
>> that lets people who want to manage memory do it and people who want it done
>> for
>> them have it done for them and neither has to deal with the other's path.
>
> I thought about this approach initially and decided against it since
> it was an extra function for every object. I'm not opposed to this and
> will add this to a list of items to send out to the mailing list about.
cool.
>> > I wanted to keep all _create and _destroy functions consistent
>> > taking only the "this". I didn't want to introduce extra parameters
>> > here. Also, it is possible to connect without any DB context, you can
>> > just set it later in the connection or possibly reference tables via
>> > database names as well (not sure if this is 100% supported in the
>> > server still, but thats the plan).
>>
>> that's not the way it would be done in a clean oo situation. the constructor
>> would take all of the required information so that the object would be valid
>> and
>> usable immediately after construction. i'm not sure of the benefit of a
>> constructor not taking any parameters. there are situations where you can't
>> pass
>> everything in on construction, but those seem to be pretty rare.
>
> The problem here is that there are multiple ways to use a connection
> object, but I don't want a set of constructor functions. If we were in
> C++ this would be fine, but in C this gets a bit ugly. I added a couple
> convenience functions, _add_tcp and _add_udp so you don't need to use
> the _create function directly and then set all the parameters. Would
> you like to see something else besides those two?
the way ldrizzle-proto's connection works is that you do
drizzle_connection_initialize(&con, &drizzle, dbname);
at that point if you're connecting to localhost without auth you can do
drizzle_connection_connect(&con)
the rest is optional, if you want to specify a host you call
drizzle_connection_set_tcp(&con, host, port);
or socket you call
drizzle_connection_set_uds(&con, socket);
if you want to specify a username and password you call
drizzle_connection_set_auth(&con, user, pass);
after initialize you have a 'ready' connection. if you want to be more specific
you call other methods to set it up.
set_tcp and set_uds are orthogonal, set_auth could apply to either.
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. 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....
> Agreed, this looks good and is a more intutive API. I'll do something
> like this when I rework query.c.
cool.
>> i looked for examples of things using libdrizzle and didn't really come up
>> with
>> much. i started out playing around with ruby bindings (i realize there are
>> other
>> projects already going after this) with an eye for the rails use case. it's
>> pretty straight-forward, essentially the simple example with a non-blocking
>> call
>> to drizzle_query_str. i quickly bound the objects to ruby, but ran in to no
>> end
>> of life-time management stuff. (which i'll get in to in the next section.)
>>
>> things like not freeing connection objects that were added to a drizzle
>> object
>> or rows of a result are completely non-obvious and should be avoided. if in
>> one
>> case i have to free the connections and in others i'm not supposed to you've
>> created more for me to think about and do in order to get things right. if i
>> start out with stand-alone connections and then decide i need to put them in
>> to
>> a drizzle object (that i own) then i have to go thought and change all of the
>> code freeing connections or else double free things.
>
> 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.
>> on the face it seems helpful to recursively free like that, but in reality it
>> creates more chances for problems. looking at the api and examples i have no
>> clue that's the designed behavior, what things i have to free myself and what
>> things will be free'd for me. i have to go in and read some documentation or
>> watch things blow up in order to figure that out.
>
> 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. 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. 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.
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.
>> the freeing behavior especially is a problem in language bindings where you
>> can
>> foresee how things will be used. you don't really have any control over when
>> objects are created and destroyed. example:
>>
>> DrizzleResult r;
>> {
>> Drizzle drizzle;
>> DrizzleConnection con;
>>
>> ...;
>>
>> r = query()
>> }
>> r.get_data();
>>
>> a somewhat simplified/made-up case, but you have an object r that will be
>> completely invalidated if the bindings got rid of con and/or drizzle when
>> they
>> went out of scope, which they'd have to do. this is going to blow up hard and
>> the DrizzleResult bindings are going to have no way of knowing that the
>> object
>> they own has been free'd out from under them.
>
> 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...
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.
>> > drizzle_con_query will block, as will all potentially blocking
>> > functions would, without NON_BLOCKING. Not sure what you mean here.
>>
>> then i'm totally confused, how do i have a clue what is and isn't blocking?
>> what
>> case would i ever want drizzle_query_str to block in? i'd much rather it
>> (non-blocking) return a query object that when i'm ready for the results will
>> block if need be. that's the 95% use case of libdrizzle, effectively
>> simple.c is
>> what most clients should look like, but i would like to see the default
>> behavior
>> start the query non-blocking and block at the point at which
>> drizzle_result_buffer is called. (which is also why i think drizzle_query_str
>> should be returning a query object that could cleanly be blocked on.)
>
> You are talking about two different things here. I use non-blocking to
> refer low-level socket I/O when you have your own custom event loop
> (like libevent). What you want is blocking calls, but for query()
> to return once the query has been sent (and before it tries to read a
> result). You can do this by setting DRIZZLE_CON_NO_RESULT_READ on the
> connection and then calling drizzle_result_read() to start reading
> the result when you want it (which will also be blocking).
i'm back to beating a dead horse at this point, but that's in no way shape or
form obvious. you can document it all you want, but i'm not going to know to
look for a connection object option to keep drizzle_query_str from blocking. and
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
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.
>> anyway, take a look at main.c (and the rest of the code from there) i think
>> it's
>> a pretty good example of what 95% of the use-cases would look like.
>> everything
>> i'm espousing is exemplified there. it's non-blocking and 'optimally'
>> buffered
>> without the client even having to know what those things mean, e.g. they get
>> the
>> optimal behavior by default.
>
> This may just come down to a difference in opinion, but I see the
> code in main.c to be a bit verbose. The examples/simple.c I think
> shows pretty much the same thing but is a bit more concise. Everyone
> will have their different habits and opinions on what the API should
> look like though. :)
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 *.
> Now, you do bring up an interesting point as to which behaviors
> are the default. I think library users generally will be waiting
> for the results immediately after the query rather than wanting to
> block for results later in the application. I know other languages
> are different and may want the async nature (ie, ruby), but that's
> what the behavior flags are for. This is probably another thing to
> throw out the to list to see what folks think.
i think with the way libdrizzle is design currently that's the default way
people will use it. if it's more complicated to do a non-blocking query send and
then get results fewer people will do it and thus the most common case won't be
as optimal as it could be. if the first example people see has a query being
sent without blocking and the results being waited on, if necessary, later than
a lot of clients will realize they can use the time in between to do some other
stuff, if nothing else send off more queries. do that and all of the sudden
you've made it a step easier (no extra work) for people to get at a more
advanced usage.
> Agreed, there are some things about it I do like. Some of it is just
> a matter of style/habit though too. For example, a bit of the ldrizzle
> code seems to be name remappings and duplicating functionality that is
> either already controlled by a lower level behavior flag or could be
> easily. I think we're actually fairly close to each other as far as how
> things should go and I'd really like to just clean-up the libdrizzle
> stuff so everyone is mostly happy. Due to style differences this may
> not be ideal for both, but I think having two low-level C APIs will
> be more confusing.
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.
> I think the best way to move forward is to start looking at specific
> libdrizzle changes to get your desired behavior. I'd really like
> to avoid the naming differences for now since there are a number of
> projects already using libdrizzle. Let me know what you think.
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. 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.
that said, i'm happy to continue to discuss specific cases etc.
> As always, thanks for the great feedback!
np, thanks yet again for listening.
best,
--
-rm
_______________________________________________
Mailing list: https://launchpad.net/~drizzle-discuss
Post to : [email protected]
Unsubscribe : https://launchpad.net/~drizzle-discuss
More help : https://help.launchpad.net/ListHelp
