Fix error in JDatabaseMySQLi::close

[elinw]

PHP handles MySQLi differently than it does MySQL specifically in a more object oriented way. For example, PHP 5 introduced a new way of handling destuctors in MySQLi (http://php.net/destruct). This patch adjusts the MySQLi driver so that when (for whatever reason) the mysqli object ($this->connection) gets destructed before the JDatabaseMysqli object does the condition check in __destruct does works expected.

[joomla-jenkins]

Unit testing complete. There were 0 failures and 0 errors from 1882 tests and 11019 assertions.
Checkstyle analysis reported 254 warnings and 1 errors.

[joomla-jenkins]

Build triggered by changes to the head.

Unit testing complete. There were 0 failures and 0 errors from 1882 tests and 11019 assertions.
Checkstyle analysis reported 254 warnings and 1 errors.

[eddieajau]

Do you have a test plan to replicate this?

[elinw]

To replicate the bug? You can just try to run Finder n mysqli with it not fixed. BTW I should say it is not a fatal error it is a Notice and it causes Finder to fail as I mentioned on the mailing list post where I raised it. Klas reports having seen it in other places too. Basically it is when you have already closed but then try to close again. It could be race conditions in some cases but it's not true in all cases. It's not a race condition in finder as far as I have been able to determine.

[eddieajau]

So, the translation of that is I have to get the Joomla CMS (trunk presumably?), then install Finder (from where?), then replace the CMS libraries with the latest platform, then … do what with Finder? I'm not trying to be difficult but "just run Finder" doesn't give me any assistance in trying to isolate the problem nor replicate the conditions where you are seeing the problem. Also, have you pushed this issue upstream to the dev's doing Finder because maybe they have a logic error in their code (because, in the absence of any better information, the fix make absolutely no sense)? If not, then that's the first port of call.

Thanks in advance.

[elinw]

I'm sorry Andrew, but I posted a discussion on list before submitting this patch. I'm sure you read the thread which discussed it and then the follow up from Klas that he had experienced the same issue. Perhaps it IS bad code, but what is wrong with putting in a check to make sure there is a connection before closing it? Given that no one had any other suggestion this is the solution that solved the problem for me. Of course if someone had suggested another approach that would have been greatly appreciated. https://groups.google.com/forum/#!topic/joomla-dev-platform/fJ0NrW56qt8

And yes of course I have pushed this "upstream" to Michel and myself and asked for advice from the original developers of Finder but no one has come up with another solution. Presumably it does not blow up in MySQL because it tests is_resource rather than is_object. There is no point in even including the is_object test since the object will always exist so we could also conceivably just drop the code but then again that might lead to other problems.

[eddieajau]

I don't like fixing code when I can't see the root cause. The problem with putting in a test that the class property exists is that it must exist by definition. In other words, the fix makes no sense at all and is just masking a deeper issue. If the connection property of the class is going away, someone or something is doing something crazy to make that happen.

Klas's case is a bit different because it involved session closing, which makes at least makes this kind of cause plausible, but no less easy to track down. And, to use his words "what happened before?". What's the backtrace on the warning? Does it happen randomly anywhere in the execution? Does it happen after a specific query?

It seems more likely that Finder's code is inadvertently closing the SQL connection somehow but possibly the DB driver is not cleaning up the connection. Soooo, on that note, you could try this:

public function __destruct()
{       
    if (is_object($this->connection))
    {
        mysqli_close($this->connection);
        $this->connection = null;
    }
}

[eddieajau]

Grasping at straws, see if Finder does anything like unset($db) anywhere, or just throw a print_debug_backtrace in the destructor and see if it's being destroyed prematurely.

[elinw]

Could just be
if ($this->get('connection') != null)
Since the object always exists.

Alternatively we could just drop the condition completely since the current test will always be true. In that case the tests in the other drivers should probably also be dropped.

[eddieajau]

Did you try what I suggested?

You don't need to use the get method because we know the class property exists. If that makes a difference (i.e., causes the warning to go away), there is something fundamentally wrong that we are missing.

[elinw]

Exactly though about the get ... I spent really a whole day trying different approaches that seemed to make more sense, but the only thing I could find was that if I dumped out get('connection') it would be null. That's NOT checking that it exists, it is checking if it actually contains anything. It exists always that is why is_object doesn't work.

I will try what you said now. I really tried to dig around tonight and the one place I found that seemed likely was this:

    // Close the application.
    JFactory::getApplication()->close();

Which is used in two different places having to do with JSON response.

[elinw]

Andrew, I think the issue is in that general area that I said, but also we have to keep in mind the mysqli functions don't always act like the similarly named mysql ones so like mysqli_connect http://ca2.php.net/manual/en/mysqli.connect.php versus http://ca2.php.net/manual/en/function.mysql-connect.php

The issue in the Finder code is in this general area

https://github.com/Finder-Integration/joomla-cms/blob/finder/administrator/components/com_finder/controllers/indexer.json.php#L259

[eddieajau]

All JObject::get does is checks if the class property is set and is not null. It's exactly the same as if ($this->connection !== null). Therefore, the only reason why the is_object would fail is if the driver connection has been severed, but $this->connection is holding an "old" object. There is nothing wrong with the is_object check as far as I can see.

JFactory::getApplication()->close() is just a proxy for exit, but that is going to kick off session saving and a whole swag of destructors.

Actually, my suggestion to nuke the connection property is inane because we are in the destruct method anyway. Maybe Finder's tripping some odd garbage collection bug in PHP?? So, I'm assuming that this is in the backend indexer code? Does it start indexing, then stop, or what? Are you seeing this error in the CMS as well, or only the ajax response (and if so, how are you seeing the error)? I really need to you to describe as much as you can about your setup and how the issue is manifesting, otherwise all I can do is throw it back and say Finder is doing something dumb.

[elinw]

So what I am seeing is that if the object exists and is null is_object is returning true not false. Is me checking != null as opposed to !== null is why it is returning false?
I think what is happening is is exactly what you said, mysqli_close only nulls out the object it doesn't destroy it (which is said in one of the comments in the manual) .

Yes this is the back end indexer and what happens is that it starts then hangs. and you get the "Warning: mysqli_close(): Couldn't fetch mysqli in /home/ian/www/finder/libraries/joomla/database/database/mysqli.php on line 160" message. http://pastebin.com/MWmv6cEV You are also right about there being a whole bunch of destructors gong on and it looks like maybe the issue is actually something in jSession.

It definitely could be finder in that there may have been a change in php 5.3
I will also say that searching on this error message shows that it is not an issue unique to Finder.

Now this it is complicated, but i went back to the build as it was on November 30 which is the data I submitted the first report

[eddieajau]

Elin, if your $this->get('connection') equates to null, then is_object($this->connection) must equate to false.

Ok, if the indexer starts then stops, I suspect then it's probably something to do with how it's iterating. The "cause" is going to be somewhere further upstream. Whether it's in the platform or not, I couldn't say at this stage (my guess is it's more to do with the session if anything). Maybe ask Rob if he's encountered this problem before in 1.5.

[eddieajau]

ZF2 does the equivalent of this (which you could try but I think you'll get the same result):

if ($this->connection instanceof \mysqli)

So, basically, the platform code is right, but something else is causing the connection to drop.

The other thing is I checked and the get method doesn't in the latest copy of the platform. The problem seems common to all web portals. I'm leaning towards it's more a server-driver problem beyond our control.

[ianmacl]

So, I looked into this issue a little bit, and I think I have an explanation. If you look at:
http://php.net/manual/en/function.mysqli-connect.php you will see it is an alias for: http://php.net/manual/en/mysqli.connect.php. This function always returns an object. If you look at http://php.net/manual/en/function.mysql-connect.php you will notice that it returns a resource.

So, important point 1: In the Mysqli class $this->connection is an object.

So next, if you look at http://php.net/destruct, you will see that it says: "PHP 5 introduces a destructor concept similar to that of other object-oriented languages, such as C++. The destructor method will be called as soon as there are no other references to a particular object, or in any order during the shutdown sequence."

In our case, we are dealing with the latter case - the destructor method can be called in any order. I was able to reproduce the issue in the CMS finder repo. To investigate what was happening, I added the following class:

class MyMysqli extends mysqli
{
public function __destruct()
{
global $dest;
$dest = true;
}
}

and then changed the constructor to (approx line 130) to: $this->connection = new MyMysqli instead of calling mysqli_connect.

Then I changed the destructor to:

    /**
     * Destructor.
     *
     * @since   11.1
     */
    public function __destruct()
    {
            global $dest;

            if ($dest) echo 'Destructed already';
            if (is_object($this->connection))
            {
                    mysqli_close($this->connection);
            }
    }

When I reran the indexer it echoed Destructed already every time before displaying the warning. If I added !$dest to the conditional the warning went away.

So this seems to be an issue when for whatever reason the mysqli object ($this->connection) gets destructed before the JDatabaseMysqli object does. I suspect we might see the same thing when we start using the PDO classes.

From my testing checking to see if it is an instance of mysqli doesn't help. The best I could come up with was is_callable($this->connection, 'close') which seemed to deal appropriately with the problem.

[eddieajau]

Ah, that makes sense. So the patch just needs to change the conditional to:

if (is_callable($this->connection, 'close'))

[elinw]

Done.
Thanks for working this through with me.

[eddieajau]

We'll just wait to see if jools is happy with that

[joomla-jenkins]

Build triggered by changes to the head.

Unit testing complete. There were 0 failures and 0 errors from 1895 tests and 11033 assertions.
Checkstyle analysis reported 207 warnings and 1 errors.

[eddieajau]

Jools is unhappy about something.

[elinw]

ok should be good now

[elinw]

Mysterious new tabs removed, pull message updated, history squashed.

[joomla-jenkins]

Build triggered by changes to the head.

Unit testing complete. There were 0 failures and 0 errors from 1895 tests and 11033 assertions.
Checkstyle analysis reported 199 warnings and 0 errors.

[eddieajau]

Thanks. Might be worth letting Nooku know since they have the same bug ;)