[Maria-developers] Freeing options & wrappers.
Hi! (sorry I forgot mailing list so resending the e-mail) So it is about https://jira.mariadb.org/browse/MDEV-10559 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. 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. 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. 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 Again, I am not 100% sure that it is possible nor really know the place were this should be added (if idea above ok, I probably find ho to make it).
Oleksandr Byelkin <sanja@montyprogram.com> writes:
https://jira.mariadb.org/browse/MDEV-10559
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?
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: https://lists.launchpad.net/maria-developers/msg04324.html And there is the knowledgebase, of course, but its mostly about how to use it from application code: https://mariadb.com/kb/en/mariadb/non-blocking-client-library/ 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);
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. 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: https://lists.launchpad.net/maria-developers/msg04324.html "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)) mysql_close_free_options(mysql); 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? 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. Or maybe force CLIENT_REMEMBER_OPTIONS on if 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 compatibility. Hope this helps, - Kristian.
Oleksandr Byelkin <sanja@montyprogram.com> writes:
https://jira.mariadb.org/browse/MDEV-10559
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:
https://lists.launchpad.net/maria-developers/msg04324.html
And there is the knowledgebase, of course, but its mostly about how to use it from application code:
https://mariadb.com/kb/en/mariadb/non-blocking-client-library/
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
Hi, Kristian! On 16.08.2016 09:07, Kristian Nielsen wrote: 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:
https://lists.launchpad.net/maria-developers/msg04324.html
"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)) mysql_close_free_options(mysql);
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. Or maybe force CLIENT_REMEMBER_OPTIONS on if 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 compatibility.
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
Hi, Kristian! On 16.08.2016 13:22, Oleksandr Byelkin wrote: [skip]
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
So what do you think about the patch above implementing: diff --git a/sql-common/mysql_async.c b/sql-common/mysql_async.c index 80b4f39..decf48e 100644 --- a/sql-common/mysql_async.c +++ b/sql-common/mysql_async.c @@ -455,7 +455,11 @@ MK_ASYNC_START_BODY( parms.db= db; parms.port= port; parms.unix_socket= unix_socket; - parms.client_flags= client_flags; + /* + async wrapper enforce the CLIENT_REMEMBER_OPTIONS flag to be + functional (otherwise it can't operate) + */ + parms.client_flags= client_flags | CLIENT_REMEMBER_OPTIONS; }, NULL, r_ptr, diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c index 640ba29..2acec60 100644 --- a/tests/mysql_client_test.c +++ b/tests/mysql_client_test.c @@ -18443,7 +18443,7 @@ static void test_bug58036() if (mysql_real_connect(conn, opt_host, opt_user, opt_password, opt_db ? opt_db : "test", - opt_port, opt_unix_socket, CLIENT_REMEMBER_OPTIONS)) + opt_port, opt_unix_socket, 0)) { if (!opt_silent) printf("mysql_real_connect() succeeded (failure expected)\n"); @@ -18469,7 +18469,7 @@ static void test_bug58036() mysql_options(conn, MYSQL_SET_CHARSET_NAME, "latin1"); if (!mysql_real_connect(conn, opt_host, opt_user, opt_password, opt_db ? opt_db : "test", - opt_port, opt_unix_socket, CLIENT_REMEMBER_OPTIONS)) + opt_port, opt_unix_socket, 0)) { if (!opt_silent) printf("mysql_real_connect() failed: %s (%d)\n", @@ -19344,7 +19344,7 @@ static void test_big_packet() if (!(mysql_real_connect(mysql_local, opt_host, opt_user, opt_password, current_db, opt_port, - opt_unix_socket, CLIENT_REMEMBER_OPTIONS))) + opt_unix_socket, 0))) { mysql_close(mysql_local); fprintf(stderr, "\n connection failed(%s)", mysql_error(mysql_local));
Oleksandr Byelkin <sanja@montyprogram.com> writes:
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.
But Elena wrote that the leak was introduced with the "Client attributes" patch, not with the async code. Why? Or was she mistaken?
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).
Right, that should at least make existing code using the async API continue to work... Of course, other code that relies on options not being deleted by failed mysql_real_connect() will still break. But hopefully there is no such code, also given that MySQL version works differently. Also, this needs to be documented carefully somewhere. It's bad enough that mysql_real_connect() magically removes options upon failure, if mysql_real_connect_start() and _cont() work differently that's even more unpleasant surprises for the user.
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
But Connector-C should have exactly the same problem. If it frees the options on failed connect, it will break async API. If it does not, code that skips mysql_close() will leak. How does C/C solve this problem? If we are making any changes in 10.0, we should preferably make things work the same as they will work in 10.2.
So what do you think about the patch above implementing:
diff --git a/sql-common/mysql_async.c b/sql-common/mysql_async.c index 80b4f39..decf48e 100644 --- a/sql-common/mysql_async.c +++ b/sql-common/mysql_async.c @@ -455,7 +455,11 @@ MK_ASYNC_START_BODY( parms.db= db; parms.port= port; parms.unix_socket= unix_socket; - parms.client_flags= client_flags; + /* + async wrapper enforce the CLIENT_REMEMBER_OPTIONS flag to be + functional (otherwise it can't operate) + */ + parms.client_flags= client_flags | CLIENT_REMEMBER_OPTIONS;
Hm, I'm not sure that is enough. It is perfectly valid (and probably usual) for applications to use the normal mysql_real_connect(), and later use eg. async mysql_real_query_start(). In this case, removing the MYSQL_OPT_NONBLOCK in mysql_real_connect() can still cause later async calls to crash. Why not just make deleting the options conditional, something like this: + if (!(client_flag & CLIENT_REMEMBER_OPTIONS) && + !mysql->options.extension->async_context) + mysql_close_free_options(mysql); Then if MYSQL_OPT_NONBLOCK is set, options will not be freed. If not set, it will work like your original patch.
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).
Yes (There are some more macro magic in the test cases, in tests/nonblock-wrappers.h. It is used to run the test in a special mode where everything uses the async functions). - Kristian.
participants (2)
-
Kristian Nielsen -
Oleksandr Byelkin