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.