1

This script resets after 100,000. What do I need to change to prevent a reset and instead keep counting?

<?php
$filename1 = 'content/general_site_data/total_site_page_loads.txt';

if (file_exists($filename1)) {
    $fh = fopen("content/general_site_data/total_site_page_loads.txt", "a+");
    if($fh==false)
        die("unable to create file");

    $filec = 'content/general_site_data/total_site_page_loads.txt';
    if (!is_writable($filec))
        die('not writable');

    $total_site_page_loads = trim(file_get_contents($filec)) + 1;
    fwrite(fopen($filec, 'w'), $total_site_page_loads);

    echo '------------------------------<br />
    Site Wide Page Views: '.$total_site_page_loads.'<br />';
} else {
    $fh = fopen($filename1, "a");
    $total_site_page_loads = trim(file_get_contents($filename1)) + 1;
    fwrite($fh, $total_site_page_loads);
    fclose($fh);

    echo '------------------------------<br />
    Site Wide Page Views: '.$total_site_page_loads.'<br />';
}
?>
poke
  • 369,085
  • 72
  • 557
  • 602
mobilestimulus
  • 180
  • 1
  • 14
  • 4
    There is nothing in this code that could cause a reset. – Pekka Mar 09 '11 at 18:03
  • @Pekka I know, right? But it seems it does it anyways. – mobilestimulus Mar 09 '11 at 18:08
  • 1
    Can you show a live example of how the count file changes when approaching 100,000? – Pekka Mar 09 '11 at 18:08
  • 1
    There is no file locking here, the second open in `w` mode performs a truncation. Classic race condition, thus the possible reset. Answer forthcoming. – Charles Mar 09 '11 at 18:09
  • 1
    @Charles but the "w" access seems to run only if the file doesn't exist – Pekka Mar 09 '11 at 18:11
  • 1
    @Pekka, yikes, the severe lack of indentation is making it hard to follow. Poking at it now... – Charles Mar 09 '11 at 18:12
  • 1
    @Charles yeah, this indeed needs better indentation – Pekka Mar 09 '11 at 18:12
  • 1
    @Pekka, the open in w *does* happen inside the if-file-exists block. He's opening it in append mode and then *reopening it* in write mode. wtf. – Charles Mar 09 '11 at 18:14
  • 1
    @Charles ugh! This indeed makes little sense. Although I *think* it should work anyway... As far as I can follow – Pekka Mar 09 '11 at 18:16
  • 1
    @mobile can you fast forward it a little? I'd hate to press F5 99,999 times :) – Pekka Mar 09 '11 at 18:22
  • @Pekka Lol! Yeah. It keeps reseting... – mobilestimulus Mar 09 '11 at 18:23
  • 1
    Code makes little sense. It defines the filename twice and uses a silly if/else. Not worth to salvage or probe for the error source. – mario Mar 09 '11 at 18:24
  • 1
    @mobile aahh. This indeed looks like a race condition. Try @Charles' solution – Pekka Mar 09 '11 at 18:24
  • 2
    @Pekka, it turns out that after going through the code, the else block should never actually even trigger -- the initial open is in append-or-create mode, so the file exists check should never fail unless the open also failed, which would cause the script to die. I've updated my answer with safer code. (I suppose this is what I get for having worked with flat file databases for so long. The horrible memories...) – Charles Mar 09 '11 at 18:25

2 Answers2

5

Your code may be suffering from a race condition.

Mid way through, you re-open the file in w mode, which truncates the file to zero length. If another copy of your script opens and attempts to read the file while it's been truncated but before it's been read, the counter will be reset to zero.

Here is an updated version of your code:

    $filename = 'content/general_site_data/total_site_page_loads.txt';
// Open our file in append-or-create mode.
    $fh = fopen($filename, "a+");
    if(!$fh)
        die("unable to create file");
// Before doing anything else, get an exclusive lock on the file.
// This will prevent anybody else from reading or writing to it.
    flock($fh, LOCK_EX);
// Place the pointer at the start of the file.
    fseek($fh, 0);
// Read one line from the file, then increment the number.
// There should only ever be one line.
    $total_site_page_loads = 1 + intval(trim(fgets($fh)));
// Now we can reset the pointer again, and truncate the file to zero length.
    fseek($fh, 0);
    ftruncate($fh, 0);
// Now we can write out our line.
    fwrite($fh, $total_site_page_loads . "\n");
// And we're done.  Closing the file will also release the lock.
    fclose($fh);
    echo '------------------------------',
         '<br />Site Wide Page Views: ',
         $total_site_page_loads,
         '<br />';

Because the initial open is in append-or-create mode, you don't need to handle a case where the file doesn't exist, unless the initial open failed.

With the file locking in place, this code should never reset the counter in the file, no matter how many concurrent requests there are. (Unless you happen to also have other code writing to the file, of course.)

Charles
  • 50,943
  • 13
  • 104
  • 142
  • 3
    Nice work. Out of curiosity, what will happen when the lock is in place - will the concurrent scripts wait, or skip? – Pekka Mar 09 '11 at 18:26
  • 3
    LOCK_EX is a blocking operation, so the script will sit and wait the few moments until things get released. If the owner of the lock exits without releasing it, proper lock implementations will release the lock. It's up to the OS and filesystem, really. There are all sorts of edge case problems here, including the operating system running out of file locks (seriously) and ancient NFS versions screwing things up if the filesystem is an NFS mount, though that seems to be a problem with really old Solaris versions on really horrible web hosts, usually run by telcos. I hated my old job sometimes. – Charles Mar 09 '11 at 18:33
  • 1
    By "the few moments," I mean microseconds. Reading, truncating, and rewriting a tiny file like this should be effectively instantaneous unless the server is severely I/O starved. Only if you're getting many, many concurrent requests will your users ever notice a delay. – Charles Mar 09 '11 at 18:34
  • 1
    (Also, this is exactly why databases are so awesome. Being able to say `UPDATE Counter SET hits = hits + 1 WHERE site = 'mine'` and letting the database engine deal with the concurrency issue is delightful.) – Charles Mar 09 '11 at 18:36
  • @Pekka and @Charles - This seems to be what answer I was looking for. Thanks for having a look, both of you. – mobilestimulus Mar 09 '11 at 19:00
  • @Pekka and @Charles - Can you two have a look at my follow-up question? - http://stackoverflow.com/questions/5251478/how-to-reset-a-persistent-counter-at-a-particular-value – mobilestimulus Mar 09 '11 at 20:45
1

I can't see where any reset would occur but how the script works seems pretty straightforward. Maybe try editing total_site_page_loads.txt to something like 99990 and watch what happens to that file as you cross over to 100000?

John
  • 15,990
  • 10
  • 70
  • 110