Skip to content

Commit da1b635

Browse files
fabpotfabpot
committed
merged branch msonnabaum/httpcache_store_locking_fixes (PR #5381)
This PR was squashed before being merged into the master branch (closes #5381). Commits ------- 0f3126f Added lockExists to Store interface, fixed locking bugs, added tests. Discussion ---------- Added lockExists to Store interface, fixed locking bugs, added tests. While working on Drupal's HttpCache implementation, I discovered that the base HttpCache class does an is_file to check for a lock, which assumes a file-based cache is being used. This seems like a mistake since the rest of the Store interface is easily swappable. I added a lockExists method so that this is properly abstracted. I also noticed there were no tests for the change I made, so I added some very basic locking tests. While adding those I found that the existing lock method is a bit broken. This line here: ```php <?php if (false !== $lock = @fopen($path = $this->getPath($this->getCacheKey($request).'.lck'), 'x')) { ``` will return false if the file couldn't be written for any reason, but the rest of the method assumes that if $lock == false, the lock exists already. So if the file couldnt be written due to the parent directory not existing, $path will be returned as if it exists, which is clearly not the desired behavior. I changed this to return false if the file couldnt be written and doesn't exist, $path if it exists, and true if the lock was created. It still doesn't feel great to have bool|string return values, but that's the best I could come up with atm. I also added a check for the parent directory that creates it if it doesn't exist. The new tests fail without it. I also broke out that code a bit as it was very difficult to read. --------------------------------------------------------------------------- by henrikbjorn at 2012-08-30T09:11:16Z Symfony have a editorconfig file which set the correct indentation settings. http://editorconfig.org/ --------------------------------------------------------------------------- by msonnabaum at 2012-08-30T13:00:20Z Updated based on stof's feedback. --------------------------------------------------------------------------- by msonnabaum at 2012-08-30T13:21:40Z Fixed based on code style feedback. --------------------------------------------------------------------------- by jonathaningram at 2012-09-05T12:29:47Z @msonnabaum, this seems to be distantly related to my recent PR too: #5376. --------------------------------------------------------------------------- by stof at 2012-10-13T20:35:55Z @fabpot anything left to merge this ? --------------------------------------------------------------------------- by catch56 at 2012-10-23T16:42:10Z This looks great to me, Couldn't find anything to complain about.
2 parents 7b998d9 + 0f3126f commit da1b635

File tree

4 files changed

+35
-4
lines changed

4 files changed

+35
-4
lines changed

‎src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -515,7 +515,7 @@ protected function lock(Request $request, Response $entry)
515515

516516
// wait for the lock to be released
517517
$wait = 0;
518-
while (is_file($lock) && $wait < 5000000) {
518+
while ($this->store->isLocked($request) && $wait < 5000000) {
519519
usleep(50000);
520520
$wait += 50000;
521521
}

‎src/Symfony/Component/HttpKernel/HttpCache/Store.php

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,15 +71,20 @@ public function cleanup()
7171
*/
7272
public function lock(Request $request)
7373
{
74-
if (false !== $lock = @fopen($path = $this->getPath($this->getCacheKey($request).'.lck'), 'x')) {
74+
$path = $this->getPath($this->getCacheKey($request).'.lck');
75+
if (!is_dir(dirname($path)) && false === @mkdir(dirname($path), 0777, true)) {
76+
return false;
77+
}
78+
79+
$lock = @fopen($path, 'x');
80+
if (false !== $lock) {
7581
fclose($lock);
7682

7783
$this->locks[] = $path;
7884

7985
return true;
8086
}
81-
82-
return $path;
87+
return !file_exists($path) ?: $path;
8388
}
8489

8590
/**
@@ -92,6 +97,11 @@ public function unlock(Request $request)
9297
return @unlink($this->getPath($this->getCacheKey($request).'.lck'));
9398
}
9499

100+
public function isLocked(Request $request)
101+
{
102+
return is_file($this->getPath($this->getCacheKey($request).'.lck'));
103+
}
104+
95105
/**
96106
* Locates a cached Response for the Request provided.
97107
*

‎src/Symfony/Component/HttpKernel/HttpCache/StoreInterface.php

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,15 @@ public function lock(Request $request);
6969
*/
7070
public function unlock(Request $request);
7171

72+
/**
73+
* Returns whether or not a lock exists.
74+
*
75+
* @param Request $request A Request instance
76+
*
77+
* @return Boolean true if lock exists, false otherwise
78+
*/
79+
public function isLocked(Request $request);
80+
7281
/**
7382
* Purges data for the given URL.
7483
*

‎src/Symfony/Component/HttpKernel/Tests/HttpCache/StoreTest.php

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,18 @@ public function testOverwritesNonVaryingResponseWithStore()
194194
$this->assertCount(2, $this->getStoreMetadata($key));
195195
}
196196

197+
public function testLocking()
198+
{
199+
$req = Request::create('/test', 'get', array(), array(), array(), array('HTTP_FOO' => 'Foo', 'HTTP_BAR' => 'Bar'));
200+
$this->assertTrue($this->store->lock($req));
201+
202+
$path = $this->store->lock($req);
203+
$this->assertTrue($this->store->isLocked($req));
204+
205+
$this->store->unlock($req);
206+
$this->assertFalse($this->store->isLocked($req));
207+
}
208+
197209
protected function storeSimpleEntry($path = null, $headers = array())
198210
{
199211
if (null === $path) {

0 commit comments

Comments
 (0)