Skip to content

Commit

Permalink
Symlink 404 cache files atomically to avoid potential race condition.
Browse files Browse the repository at this point in the history 
See #242
  • Loading branch information
@raamdev
raamdev committed Aug 6, 2014
1 parent f012c36 commit 1c11f59
Showing 1 changed file with 11 additions and 2 deletions.
13 changes: 11 additions & 2 deletions quick-cache/includes/advanced-cache.tpl.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -909,8 +909,10 @@ public function output_buffer_callback_handler($buffer, $phase)

# This is where a new 404 request might be detected for the first time; and where the 404 error file already exists in this case.

$cache_file_symlink_tmp = $this->cache_file.'.'.uniqid('', TRUE).'.tmp'; // Symlink creation is atomic; e.g. tmp file w/ rename.

This comment has been minimized.

Sign in to view

Copy link
@jaswrks

jaswrks Aug 6, 2014

@raamdev I see that we have $cache_file_symlink_tmp and $cache_file_tmp also. Both of these are actually the same value if I'm seeing this right; so maybe we could just bump $cache_file_tmp up a little higher in this routine so it's available when we need it (in either scenario), thereby saving the extra call to uniqid(). Not that it's a performance hit, but anything here that we can do for speed would be cool.

This comment has been minimized.

Sign in to view

Copy link
@raamdev

raamdev Aug 6, 2014

Author Contributor

Both of these are actually the same value

Actually they're the opposite of the same value... uniqid() ensures that. I used separate temp files because I didn't want there to be any chance of a conflict, but looking at the code again I see that if the symlink is created, the rest of the code doesn't run anyway. So I agree with reducing this to just $cache_file_tmp. Thanks! :)

This comment has been minimized.

Sign in to view

Copy link
@jaswrks

jaswrks Aug 6, 2014

Ha, that's true. I was just looking at the code, but definitely different. lol

if($this->is_404 && is_file($this->cache_file_404))
if(!symlink($this->cache_file_404, $this->cache_file))
if(!(symlink($this->cache_file_404, $cache_file_symlink_tmp) && rename($cache_file_symlink_tmp, $this->cache_file_404)))

This comment has been minimized.

Sign in to view

Copy link
@jaswrks

jaswrks Aug 6, 2014

@raamdev On this line I think the second argument to rename() is wrong.

This comment has been minimized.

Sign in to view

Copy link
@raamdev

raamdev Aug 6, 2014

Author Contributor

@jaswsinc Yep, I thought I changed all those, but it looks like I missed this one. Thanks!
throw new \exception(sprintf(__('Unable to create symlink: `%1$s` » `%2$s`. Possible permissions issue (or race condition), please check your cache directory: `%3$s`.', $this->text_domain), $this->cache_file, $this->cache_file_404, QUICK_CACHE_DIR));
else return (boolean)$this->maybe_set_debug_info($this::NC_DEBUG_1ST_TIME_404_SYMLINK);

Expand All @@ -924,15 +926,22 @@ public function output_buffer_callback_handler($buffer, $phase)
($this->is_404) ? '404 [error document]' : $this->salt_location, $total_time, date('M jS, Y @ g:i a T'))).' -->';
$cache .= "\n".'<!-- '.htmlspecialchars(sprintf(__('This Quick Cache file will auto-expire (and be rebuilt) on: %1$s (based on your configured expiration time).', $this->text_domain), date('M jS, Y @ g:i a T', strtotime('+'.QUICK_CACHE_MAX_AGE)))).' -->';
}

$cache_file_tmp = $this->cache_file.'.'.uniqid('', TRUE).'.tmp'; // Cache creation is atomic; e.g. tmp file w/ rename.

/*
* This is NOT a 404, or it is 404 and the 404 cache file doesn't yet exist (so we need to create it).
*/
if($this->is_404) // This is a 404; let's create 404 cache file and symlink to it.
{
if(file_put_contents($cache_file_tmp, serialize($this->headers_list()).'<!--headers-->'.$cache) && rename($cache_file_tmp, $this->cache_file_404))
if(symlink($this->cache_file_404, $this->cache_file)) // If this fails an exception will be thrown down below.
{
$cache_file_symlink_tmp = $this->cache_file.'.'.uniqid('', TRUE).'.tmp'; // Symlink creation is atomic; e.g. tmp file w/ rename.
if(!(symlink($this->cache_file_404, $cache_file_symlink_tmp) && rename($cache_file_symlink_tmp, $this->cache_file)))
throw new \exception(sprintf(__('Unable to create symlink: `%1$s` » `%2$s`. Possible permissions issue (or race condition), please check your cache directory: `%3$s`.', $this->text_domain), $this->cache_file, $this->cache_file_404, QUICK_CACHE_DIR));
else
return $cache; // Return the newly built cache; with possible debug information also.
}

} // NOT a 404; let's write a new cache file.
else if(file_put_contents($cache_file_tmp, serialize($this->headers_list()).'<!--headers-->'.$cache) && rename($cache_file_tmp, $this->cache_file))
Expand Down

0 comments on commit 1c11f59

Please sign in to comment.