diff --git a/CHANGELOG.md b/CHANGELOG.md index b3ff7162b..6a288222f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,11 +5,13 @@ * Grav 1.6: Renamed `$grav['users']` service to `$grav['accounts']` 1. [](#improved) * Renamed `Grav\Framework\File\Formatter\FormatterInterface` to `Grav\Framework\File\Interfaces\FileFormatterInterface` + * Improved `File::save()` not to use file lock, but a temporary file 1. [](#bugfix) * Grav 1.6: Fixed `FlexUser` loosing ACL information * Grav 1.6: Fixed `FlexUser::find()` breaking when nothing is found * Grav 1.6: Fixed `FlexObject::update()` removing fields on save * Fixed `mkdir(...)` race condition + * Fixed `Obtaining write lock failed on file...` # v1.6.0-rc.3 ## 02/18/2019 diff --git a/system/src/Grav/Framework/File/AbstractFile.php b/system/src/Grav/Framework/File/AbstractFile.php index f1caf0fc7..34897368f 100644 --- a/system/src/Grav/Framework/File/AbstractFile.php +++ b/system/src/Grav/Framework/File/AbstractFile.php @@ -199,8 +199,13 @@ class AbstractFile implements FileInterface throw new \RuntimeException("Opening file for writing failed on error {$error['message']}"); } } + $lock = $block ? LOCK_EX : LOCK_EX | LOCK_NB; - return $this->locked = $this->handle ? flock($this->handle, $lock) : false; + + // Some filesystems do not support file locks, only fail if another process holds the lock. + $this->locked = flock($this->handle, $lock, $wouldblock) || !$wouldblock; + + return $this->locked; } /** @@ -213,10 +218,12 @@ class AbstractFile implements FileInterface if (!$this->handle) { return false; } + if ($this->locked) { flock($this->handle, LOCK_UN); $this->locked = false; } + fclose($this->handle); $this->handle = null; @@ -275,27 +282,24 @@ class AbstractFile implements FileInterface */ public function save($data): void { - $lock = false; - if (!$this->locked) { - // Obtain blocking lock or fail. - if (!$this->lock()) { - throw new \RuntimeException('Obtaining write lock failed on file: ' . $this->filepath); - } - $lock = true; + $filepath = $this->filepath; + $dir = $this->getPath(); + + // Create file with a temporary name and rename it to make the save action atomic. + $tmp = tempnam($dir, basename($filepath)); + if ($tmp && @file_put_contents($tmp, $data) && @rename($tmp, $filepath)) { + @chmod($filepath, 0666 & ~umask()); + } elseif ($tmp) { + @unlink($tmp); + $tmp = false; } - // As we are using non-truncating locking, make sure that the file is empty before writing. - if (@ftruncate($this->handle, 0) === false || @fwrite($this->handle, $data) === false) { - $this->unlock(); - throw new \RuntimeException('Saving file failed: ' . $this->filepath); - } - - if ($lock) { - $this->unlock(); + if ($tmp === false) { + throw new \RuntimeException('Failed to save file ' . $filepath); } // Touch the directory as well, thus marking it modified. - @touch($this->getPath()); + @touch($dir); } /** diff --git a/system/src/Grav/Framework/Flex/FlexIndex.php b/system/src/Grav/Framework/Flex/FlexIndex.php index 71278c3ca..4b05dcd63 100644 --- a/system/src/Grav/Framework/Flex/FlexIndex.php +++ b/system/src/Grav/Framework/Flex/FlexIndex.php @@ -558,6 +558,7 @@ class FlexIndex extends ObjectIndex implements FlexCollectionInterface, FlexInde static::onChanges($index, $added, $updated, $removed); $indexFile->save(['count' => \count($index), 'index' => $index]); + $indexFile->unlock(); return $index; }