eventio / bbq

Message Queue abstraction library for PHP
28 stars 12 forks source link

DirectoryQueue stampede #2

Open GDmac opened 7 years ago

GDmac commented 7 years ago

There is a really small window where a stampede may cause a PHP warning just before BBQ can throw its own Rename exception. https://github.com/eventio/bbq/blob/master/src/Eventio/BBQ/Queue/DirectoryQueue.php#L42

When getting the real path (//1) and then rename (//3), the file might already be snatched by another fetchjob().

$filePath = $file->getRealPath(); // 1
$runFilePath = $filePath . '.run';  // 2
// Renaming to lock the file
if (false === rename($filePath, $runFilePath)) { // 3
  throw new \RuntimeException('Renaming from ' . $filePath . ' to ' . $runFilePath . ' failed.');
}

I am using a solution with a ".lockfile" per queue directory. Since walking and fetching the directory for an open job is a fast iteration, i am currently using a blocking lock. As an alternative the (currently unused) $timeout can be used and then do a couple of .5 second retries until $timeout.

I can make a pull-request if you like?

$lockFile = $this->directory . '/' . '.lockfile';
$lockHandle = fopen($lockFile, 'c+b');
if ($lockHandle === false) {
  throw new \RuntimeException('Cannot create the lockfile for task directory');
}
$lock = flock($lockHandle, LOCK_EX);  
// ...
flock($lockHandle, LOCK_UN);

Note: the createIterator() also has to be changed and has to not only exclude isDot() but any dotfile

protected function createIterator()
{
  $directoryIterator = new \DirectoryIterator($this->directory);
  return new \CallbackFilterIterator($directoryIterator, function($fileInfo) {
        return strpos($fileInfo->getFileName(), '.') !== 0;
  });
}
GDmac commented 7 years ago

additionally, i guess pushJob() should also make the file write really atomic (given that the directory iterator skips dotfiles)

file_put_contents('.' . $filename, serialize($payload));
rename('.' . $filename, $filename);