1

I'm maintaining a project based on legacy code that was written by an external company. We are in the process of fixing a lot of PHP errors in this code base and one of them comes from a call to mysqli::reap_async_query() :

mysqli::reap_async_query(): Connection not opened, clear or has been closed

This happens only on the first call to this function:

    function mysqlQuery($sql, $getLastId = false, $die = true, $alwaysDie = false, $async = false)
    {
        @$GLOBALS['con']->reap_async_query(); // This is what is triggering the error
    
        if ($async) {
            $result = $GLOBALS['con']->query($sql, MYSQLI_ASYNC);
        } else {
            $result = $GLOBALS['con']->query($sql);
        }
        if (!$result) {
            if ($alwaysDie or ($_ENV['ENV_MODE'] === 'dev' and $die)) {
                die('Etwas stimmte mit dem Query nicht: ' . $GLOBALS['con']->error . '<br/>Ganzes Query: ' . $sql);
            }
            return false;
        } else {
            $lastId = mysqlLastId();
            if ($getLastId == true) {
                return $lastId;
            } else {
                return $result;
            }
        }
    }

Note that $GLOBALS['con'] is an instance of mysqli. The code is calling mysqlQuery() with no specific parameter:

$result = mysqlQuery("SELECT * FROM someTable");

According to the Git history, the call to @$GLOBALS['con']->reap_async_query(); was added to support async SQL queries. But reading the PHP doc, it doesn't seem to be useful here at all since we are not storing its return value.

So my question is: is there a reason for it to be here, does calling it even without reading its return value have any important side effect ? I might just remove this call completely if it is useless.

Also, why is it triggering this error ? I understand that trying to read a result before any query has been executed could trigger an error but the error indicates that the connection is not active, which does not seem to be the case.

Derek
  • 1,826
  • 18
  • 25
  • It is a very bad idea to use `die(mysqli_error($conn));` in your code, because it could potentially leak sensitive information. See this post for more explanation: [mysqli or die, does it have to die?](https://stackoverflow.com/a/15320411/1839439) – Dharman Oct 29 '21 at 11:44
  • i know, i didn't write that code. This is legacy code that was written by an external partner and now we have to clean it up piece by piece. – Derek Oct 29 '21 at 12:29

2 Answers2

1

Is there a reason for it [mysqli::reap_async_query()] to be here, does calling it even without reading its return value has any important side effect ?

The return value is not assigned to a local variable, however it is still returned.

So the original interest was in calling that function. And for what for, has been written about in the commit message.

According to the Git history, the call to @$GLOBALS['con']->reap_async_query(); was added to support async SQL queries.

Let's consider this example:

$con->query('SELECT SLEEP(5) as `zzzZZZ...Schnarch...Schmatz..zzz`', MYSQLI_ASYNC);

$con->reap_async_query();

How long does it take to execute this code?

This is the reason how that call supports async queries. If an async query would still run on the connection (it has not been reaped yet), every new query would fail.

So add of the call in-so-far supports async SQL queries as it allows to fire one on the same (shared) connection that might be in use for other queries, too.


Additionally you ask:

Also, why is it triggering this error ? I understand that trying to read a result before any query has been executed could trigger an error but the error indicates that the connection is not active, which does not seem to be the case.

Let's take a look at the error, actually a message on the diagnostic channel:

PHP Warning: mysqli::reap_async_query(): Connection not opened, clear or has been closed in ...

As we know the connection has been opened and has not been closed, the last point might be it:

[...] Connection [...] clear [...]

Now I have not programmed that error message, but my reading of it is that there is no async query running yet on the (open) connection - the connection is clear.

It produces a warning as this might not be intended (there is no need to reap a clear connection normally) and henceforth as with your function this is intended, the call is prefixed with the error suppression operator (@).

hakre
  • 193,403
  • 52
  • 435
  • 836
  • thank you. I guess i'll add a flag in the function to check if a previous async query has been executed before calling reap_async_query to avoid triggering the error then. – Derek Oct 29 '21 at 09:48
  • @Derek: I edited for the error part. It's not an error, it's a diagnostic message and checking for a clear connection first will give you more side-effects normally than using the @ suppression operator. Someone might have told you get rid of all diagnostic messages, but that is not making the code better (out of its own). In a place like this which is I/O bound, remote system interaction, it's good to design for error, and here especially it makes the code speaking. There is only a bit of understanding missing, but that's standard learning experience while programming. – hakre Oct 29 '21 at 09:54
  • the issue is that the whole code we have to work on is full of @ *everywhere*, including in parts of the code that we can't alter (obfuscated code from the CMS). So we registered our own error handler to be able to actually debug anything because no errors were shown anywhere. I know that in some cases using @ is actually OK and i would be fine with it if it wasn't abused in the rest of the project. – Derek Oct 29 '21 at 09:59
  • yes, overuse of @ may lead to a one-size-fits it all approach, but as the phrase goes: [programming is hard, let's go shopping](https://blog.codinghorror.com/programming-is-hard-lets-go-shopping/). Take care and good luck! – hakre Oct 29 '21 at 10:28
  • thank you, i edited my question with the fix i plan to implement. Not sure if i should set $asyncPending back to false after the call but i'll test it! – Derek Oct 29 '21 at 10:35
  • 1
    @Derek: Instead of editing your question, place it as an answer below. It is also easier to comment on it then (and also how SO works best). – hakre Oct 29 '21 at 10:52
  • i hesitated but yes you are right, i just did that instead. – Derek Oct 29 '21 at 12:32
0

Thanks to @hakre's answer i understand that this call is there to get rid of the queue of async queries and i plan to fix it with a static flag to check if one is pending before calling @$GLOBALS['con']->reap_async_query():

function mysqlQuery($sql, $getLastId = false, $die = true, $alwaysDie = false, $async = false)
{
    static $asyncPending = false;

    if (true === $asyncPending) {
        @$GLOBALS['con']->reap_async_query();
        $asyncPending = false;
    }

    if ($async) {
        $result = $GLOBALS['con']->query($sql, MYSQLI_ASYNC);
        $asyncPending = true;
    } else {
        $result = $GLOBALS['con']->query($sql);
    }
    if (!$result) {
        if ($alwaysDie or ($_ENV['ENV_MODE'] === 'dev' and $die)) {
            die('Etwas stimmte mit dem Query nicht: ' . $GLOBALS['con']->error . '<br/>Ganzes Query: ' . $sql);
        }
        return false;
    } else {
        $lastId = mysqlLastId();
        if ($getLastId == true) {
            return $lastId;
        } else {
            return $result;
        }
    }
}
Derek
  • 1,826
  • 18
  • 25
  • As commented before introducing the if can easily introduce more - not less - side-effects and with the static also state. As the original code shows, it's not the place where the async queries are normally reaped. So you might still see errors with your changes, this time not even suppressed. Just saying. It likely will still work, but it's just easy to introduce more rough edges than it previously had. Take care. --- And Perhaps no/very little async queries are in use at all? Maybe split the function and have a dedicated one for async only. – hakre Oct 29 '21 at 12:49
  • well since that error occurs only on the very first call of the function i could use the flag just for that. Detect the first call and prevent doing reap_async_query when there is nothing to reap, but keep it enabled for any future call. I considered splitting in 2 functions but there is a lot of code to change for that... – Derek Oct 29 '21 at 14:04
  • But doesn't it mean that async queries are not in use (or perhaps the opposite: always) after all? You know the code base better than I, but there is perhaps some more insight you can take with that change. Just saying, hiding the diagnostic messages can as well hide far more import design problems that could give far more benefit. Within that function alone - without introducing anything new, there are a couple of code improvements possible out of the box as well. If you're using Phpstorm, checkout the EA inspections plugin. – hakre Oct 29 '21 at 21:13
  • yes there are definitely many improvements that can be done here but our improvements todo list is gigantic on this project. PhpStorm has half the project's code underlined for different reasons so for now we focus on identifying the cause of PHP errors and that is quite a big task already :-/ – Derek Nov 01 '21 at 09:45
  • Yes, the blinking christmas tree when you look on it. FYI: In Phpstorm you can run a specific inspection across the whole tree and then apply fixes on the tree or subtrees with a single click. Start with the no-brainers, commit, push to CI, deploy, next. If not an option, disable the inspections you're not making use of as they then create too much noise. And checkout structural search and replace. – hakre Nov 01 '21 at 11:54