maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Oleksandr Byelkin <sanja@xxxxxxxxxxxxxxxx> 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.