← Back to team overview

maria-developers team mailing list archive

Re: Freeing options & wrappers.


Hi, Kristian!

On 16.08.2016 09:07, Kristian Nielsen wrote:
Oleksandr Byelkin <sanja@xxxxxxxxxxxxxxxx> writes:


Original problem was that database drivers in Python & Perl (probably
other) do not call mysql_close for MYSQL object if there was no
connection happing (memory leak).

MySQL has solution for this - freeing option on failed connection if
user did not asked something else (we even has the same flag, but it did
not work). So I took MySQL solution.
Hm, so let me see if I understand correctly: This is actually MDEV-10455?
Which is a regression introduced by b803b46dac40d2417e41c778938d97951ce4ec88
"Client attributes"?

And you want to make a change (in stable 10.0?) to re-activate the
CLIENT_REMEMBER_OPTIONS flag, _off_ by default?
yes, that is what needed by Python & Perl.

But it appeared not so compatible with our async wrappers. Frankly
speaking the code is difficult to read and debug because everything made
over macros nor I have found decent description/overview of how it is
done (thank you if somebody point to it) so I can not say that
understand what it is doing, and all my suggestion based on how I
understood it.
There is a mailing list thread discussing this:


And there is the knowledgebase, of course, but its mostly about how to use
it from application code:


Basically, each wrapper runs the corresponding base function in a
co-routine, with its own stack stored in mysql->options.extension (similar
to running in a separate thread). So if a base function were to de-allocate
mysql->options.extension, it will destroy its own CPU stack, which I think
explains crashes and lack of stack traces...

I think the essential point here is that all use of async functions requires
first setting the MYSQL_OPT_NONBLOCK option, otherwise they will crash:

   mysql_options(&mysql, MYSQL_OPT_NONBLOCK, 0);
Huh! Interesting finding. actually test which crash to not set the flag, so do not use async operations, but wrapper is in act (by global setup probably).

So what we have: we allocate memory for some options set between
mysql_init and mysql_real_connect, instead of "reall connect" called
some wrapper which store some pointers to the options then call
mysql_real_connect and if "real_connect" free them it fail on exit.
Might fail even earlier, as real_connect would be freeing the memory of its
own CPU stack, which can never be a good idea...

I think that best solution is that wrapper

1) store current state of CLIENT_REMEMBER_OPTIONS

2) set the flag

3) call mysql_real_connect

4) if connect fail and there was not "remember options" then wrapper
free options by itself
The problem with this is that this will probably break existing programs.
Exactly, that was my concern, that is why I start asking.
If I understand you correctly, this will effectively unset the
MYSQL_OPT_NONBLOCK option on a failed mysql_real_connect() ?
If so, then any further use of an async function will immediately crash
anyway. Like mysql_close_start()...

It would basically be required for the application to immediately
re-activate the MYSQL_OPT_NONBLOCK option after every failed connect. It
seems very likely that there could be existing programs that do not do
this. So it does not seem a possible route?

If you read Monty's original review comments for the async feature, you will
find this:


   "The reason for not using mysql->options.extension was only becasue
   mysql_real_connect() freed the options on failure. This code is not needed
   as they will be freed by mysql_close() anyway.

   So I suggest you remove the code:

     if (!(client_flag & CLIENT_REMEMBER_OPTIONS))

   from mysql_real_connect()"

So that is the reason for the original removal/disabling of
CLIENT_REMEMBER_OPTIONS. I'm wondering if it is really a good idea to
re-introduce it, and even worse, do so in a stable release?
Do you have other solution for memory leak on failed connect? Of course one could rewrite python & perl, but who will?

It seems the dilemma here is that on the one hand, not supporting
CLIENT_REMEMBER_OPTIONS creates incompatibility with the MySQL version of
the library. On the other hand, supporting it breaks async-using code.
The documentation (MySQL one) does not say that mysql_close() can be omitted
on failed connect, though neither does it say that it cannot.

Elena wrote that the original regressions was introduced by a patch for
"Client attributes". What is the reason for that? I'm thinking perhaps
"Client attributes" made it so that mysql->options.extension is _always_
allocated? Then missing mysql_close() will leak the options memory, while
before the patch, programs that did not set options themselves would not
leak? If this is how things are, then another option could be to fix the
"Client attributes" patch to not allocate the extra memory by default (if
that is possible).

Other possible options could be to clear all options _except_ the
MYSQL_OPT_NONBLOCK is set. Both these options seem unattractive, since they
make the semantics even more complicated of a part of the API that is
already too magic.

In any case, I think before discussing the technical solution, it is
necessary to first carefully consider and decide on the semantics of how
things should work. Changing behaviour of the client library is very
delicate, there are a _lot_ of possibilities for breaking backwards

Hope this helps,

Thank you a lot that really helps.

So as I see it: bringing in act the wrappers makes it incompatible with the flag, but we need it to non-leaking slippery written code of connect in other cases. IMHO we just can enforce the flag in case of wrappers, so client either writes something on his knee or use async operation (where some rules must be obeyed).

Making something more I think do not worth it because:
1) in 10.2 C/C will replace the library and it has no the problem
2) we should no make huge changes before 10.2

Follow ups