4
\$\begingroup\$

I am trying to implement a live chat for my websites visitors. I am currently using a PHP script that processes in background and that is called by AJAX. I am not familiar with sockets and node.js that also allow to create a live chat so I tried to make one longpolling-like code upon my possibilities. What I am hoping for is to hear your ideas about this code, how I can improve it and what are the pros and cons for using my code.

PHP script (used for getting all the latest messages):

require ($_SERVER['DOCUMENT_ROOT'].'/includes/config.php');
$time = $_POST['time'];
$x = 0;
$allowloop = true;
while (($x < 99) && ($allowloop))
{
    if ($time == 0)
    {
        $query = mysqli_query($con, 'SELECT * FROM (SELECT * FROM chat_messages WHERE send_time > "'.$time.'" ORDER BY chat_message_id DESC LIMIT 30) T1 ORDER BY chat_message_id ASC');
    } else {
        $query = mysqli_query($con, 'SELECT * FROM chat_messages WHERE send_time > "'.$time.'"');
    }

    if ((!$query) && ($time == 0))
    {
        $allowloop = false;
    } elseif (mysqli_num_rows($query) == 0) {
        usleep(200000);
        $x = $x + 1;
    } else {
        $allowloop = false;
    }
}

if (!$allowloop)
{
    while ($messages = mysqli_fetch_assoc($query))
    {
        echo
    '           <tr id="message_'.$messages['chat_message_id'].'">
                    <td id="message_text"><div id="message_body">'.$messages['chat_message_text'].'</div></td>
                    <td id="message_time">'.date('G:i:s', $messages['send_time']).'</td>
                    <td id="time" class="hidden">'.$messages['send_time'].'</td>
                </tr>
    ';
    }
}
}

JS function that calls the code above in the background:

$('#world_chat_input').focus();
var time = 0;
var data = '';
(function chat_get() {
    time = $('table#w_messages_t tr:last td#time').html();
    data = {time: time};
    $.ajax({
        type: 'POST',
        url: '/guild/character/guild/home/chat_get.php',
        data: data,
        async: true,
        success: function (data) {
            $('#w_messages_t tbody').append(data);
            if (data) {
                $('#w_m_table_wrap').animate({
                    scrollTop: $('#w_m_table_wrap').prop('scrollHeight')
                }, 1000);
            }
            setTimeout(chat_get);
        }
    });
})();

HTML chat table:

    <table id="w_messages_t">
        <tr>
            <td id="welcome" colspan="5">Welcome to World chat</td><td id="time" class="hidden">0</td>
        </tr>
    </table>
\$\endgroup\$
1
\$\begingroup\$
$allowloop = true;
while (($x < 99) && ($allowloop))
{
// More code was here. //
    if ((!$query) && ($time == 0))
    {
        $allowloop = false;
    } elseif (mysqli_num_rows($query) == 0) {
        usleep(200000);
        $x = $x + 1;
    } else {
        $allowloop = false;
    }
}

if (!$allowloop)
{

You don't need $allowloop. You can replace these lines with

while ( $x < 99 ) {
// The code that was here stays the same. //
    if ( ! $query && ( 0 == $time ) ) {
        break;
    } elseif ( ! $query || ( 0 == $query->num_rows ) ) {
        usleep(200000);
        ++$x;
    } else {
        break;
    }
}

if ( $x < 99 ) {

I changed $x = $x + 1 to the more standard ++$x which does the same thing.

I also optimized your elseif so that it won't call mysqli_num_rows unnecessarily. And changed it so that it uses the shorter object-oriented notation.

This hits the database every .2 seconds for almost twenty seconds. Are you really in that much of a hurry? Also, be careful of timeouts. If your HTTP connection times out after five seconds, there's not much point in continuing to poll the database.

Note that if you have a lot of people chatting but no one saying anything, there will be a bunch of people doing this at once. You may want to look into solutions that allow for sharing these results among multiple HTTP connections. For example, something that caches the last two message times and returns a cached result for times between them (older results would be returned from the database). APC would work for a single server or a shared memcache for multiple servers.

    $query = mysqli_query($con, 'SELECT * FROM chat_messages WHERE send_time > "'.$time.'"');

There's a security issue here. Assuming $time is an integer, you should say,

    $query = mysqli_query($con, 'SELECT * FROM chat_messages WHERE send_time > "'.(int)$time.'"');

This avoids the possibility of SQL injection by someone who spoofs your AJAX call.

Or use mysqli_prepare (preferred).

    $statement = $con->stmt_init();
    if ($statement->prepare('SELECT * FROM chat_messages WHERE send_time > ?') ) {
        $statement->bind_param('i', $time);

Be careful about using SELECT *. It's one thing if the three columns that you use are the only columns. It's another thing entirely if you have a large text column that you don't need in the table. There are some people who believe that you should never use * and always specify the columns.

Note that using * may also cause you to go to disk when you otherwise would have a covering index. This means that even a small column can be a problem.

I don't like the name $con for the database connection. It's too short to be obvious what it is. Admittedly, $con is slightly more accurate than $db, but $db is accurate enough, reasonably standard, and more easily recognizable in my opinion.

You may want to replace send_time with chat_message_id as your check. I.e. instead of checking the time, check for newer chat_message_id values. Assuming this is an autoincrement primary key, you should already have a unique index on it. You could put an index on send_time, but it will be a slightly less performant non-unique index.

This also avoids the problem with near simultaneous chat messages where you only have the first one and never look for the second because it has the same time.

if ($time == 0)
{
    $query = mysqli_query($con, 'SELECT * FROM (SELECT * FROM chat_messages WHERE send_time > "'.$time.'" ORDER BY chat_message_id DESC LIMIT 30) T1 ORDER BY chat_message_id ASC');

This seems more complicated than it needs to be. We know that $time == 0, so why use $time in the query? Are there negative times in the table? Why not just

if ( 0 == $time ) {
    $query = $con->query('SELECT * FROM (SELECT * FROM chat_messages ORDER BY chat_message_id DESC LIMIT 30) T1 ORDER BY chat_message_id ASC');

Also switched to the shorter object-oriented notation.

Switched the order of the comparison to 0 == $time because this will throw a compile error if you leave off an = where $time = 0 will silently change the value of $time and evaluate as false. Consistently using this order will therefore make it easier to catch that particular typo.

You switch between having the { on the same line and on a new line. Either is a valid style, but please pick one and stick with it. My preference is for on the same line, but both are common.

Scaling

My primary concern with this code is that it won't scale well. You have every user in the chat polling to see if there is new data. For ten users, this will probably work fine. For a thousand users, this seems like it would put considerable load on the database. Not so much from the queries as maintaining a thousand simultaneous database connections for twenty seconds.

As a general rule, a callback is better than polling. You can generate the callback only when required whereas polling has to continually check. PHP is probably the wrong language for implementing a callback model though. You could add an additional layer between PHP and the database to fix this. PHP would call your service, which would then only call the database once (or not at all if the service also manages sending a chat message) rather than once per client.

In that model, one database connection is sufficient regardless of the number of clients. You'd only need multiple database connections if there were multiple chat channels, and even then they could share. Some languages (e.g. Java) have connection pooling so as to share database resources transparently to the client. PHP seems the wrong language to manage something like this, as it is optimized for HTTP request/reply connections. PHP's not persistent, while this problem space is.

This will probably work for a small scale application, but if it grows, you'll likely have to dump the current method and swap it out for something more robust.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.