Skip to content

Loading…

Fix #156: Only flock if non-blocking and we actually obtain the lock #245

Closed
wants to merge 4 commits into from

5 participants

@johnbellone

The issue starts off regarding NFS file systems not supporting locking, but this is really careful programming. Since it is a debug log we might not want to actually still attempt to write (I left it in there) but went over the documents at http://php.net/manual/en/function.flock.php and it seems this is the correct way to go.

@philsturgeon

Changelog please :)

@kenjis

This is danger in both NFS and non-NFS file system.
if another process is writing log, the other process also can try to write log not waiting to get lock, so it may breaks log file.

@johnbellone

From what I understood with the NB flag it will return immediately if it can't obtain the lock, no? We should probably append the pid or something to the end of each log.

@johnbellone johnbellone reopened this
@kenjis

Yes. NB does not wait, if it can't obtain the lock, return FALSE.

So your pull request causes that more than 2 process may write the same file at the same time.
I think it is better to separate logic NFS and non-NFS, and in NFS a way of locking or PID or something is needed.

See: http://www.php.net/manual/en/function.flock.php#82521

@cryode

@johnbellone Any chance you can update this issue or pull request? Has this been fixed by someone else? Let's get this finalized finally.

@johnbellone

What's our course here?

Separating the logic for NFS vs. non-NFS - a suggested method there is using a link() and stat() method of counting - this can also return FALSE on antiquated file systems such as FAT.

@narfbg

I just tested this and it's not a viable solution - it would skip logging even when it would usually be possible.

@narfbg narfbg closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 21, 2011
  1. @johnbellone
  2. @johnbellone

    Once again being stylish.

    johnbellone committed
Commits on Aug 22, 2011
  1. @johnbellone
  2. @johnbellone

    Removing write; need to figure something out regarding naming behavio…

    johnbellone committed
    …r from difference processes.
Showing with 13 additions and 3 deletions.
  1. +12 −3 system/libraries/Log.php
  2. +1 −0 user_guide/changelog.html
View
15 system/libraries/Log.php
@@ -98,9 +98,18 @@ public function write_log($level = 'error', $msg, $php_error = FALSE)
$message .= $level.' '.(($level == 'INFO') ? ' -' : '-').' '.date($this->_date_fmt). ' --> '.$msg."\n";
- flock($fp, LOCK_EX);
- fwrite($fp, $message);
- flock($fp, LOCK_UN);
+ if (flock($fp, LOCK_EX | LOCK_NB))
+ {
+ fwrite($fp, $message);
+ flock($fp, LOCK_UN);
+ }
+ else
+ {
+ fclose($fp);
+
+ return FALSE;
+ }
+
fclose($fp);
@chmod($filepath, FILE_WRITE_MODE);
View
1 user_guide/changelog.html
@@ -108,6 +108,7 @@
<li class="reactor">Fixed a bug where not setting 'null' when adding fields in db_forge for mysql and mysqli drivers would default to NULL instead of NOT NULL as the docs suggest.</li>
<li class="reactor">Fixed a bug where using <kbd>$this->db->select_max()</kdb>, <kbd>$this->db->select_min()</kdb>, etc could throw notices. Thanks to w43l for the patch.</li>
<li class="reactor">Replace checks for STDIN with php_sapi_name() == 'cli' which on the whole is more reliable. This should get parameters in crontab working.</li>
+ <li class="reactor">Fixed issue #156: Adding check for flock with non-blocking flag to fix NFS file systems that cannot lock.</li>
</ul>
<h2>Version 2.0.2</h2>
Something went wrong with that request. Please try again.