[HttpKernel] clearstatcache() so the Cache sees when a .lck file has been released #16312

mpdude · 2015-10-22T10:17:12Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#15813
License	MIT
Doc PR	n/a

I've been trying to debug #15813 and modified the Store in a way to keep unique request IDs in the .lck file. That way, I was hoping to find out which request is blocking and/or if the request is actually still running.

It turned out that is_file() would claim that a lock file still exists, but a subsequent attempt to read the information from that file returned "file not found" errors.

So, my assumption is that the is_file() result is based on the fstat cache and wrong once a process has seen the lock file.

@jakzal said in #15813 (comment) that unlink()ing the lock file should clear the statcache, but I doubt this is true across PHP processes.

aitboudad · 2015-10-22T11:07:51Z

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

@@ -106,7 +106,10 @@ public function unlock(Request $request)

    public function isLocked(Request $request)
    {
-        return is_file($this->getPath($this->getCacheKey($request).'.lck'));
+        $path = $this->getPath($this->getCacheKey($request).'.lck');
+        clearstatcache($path);


the file path should be passed into the second args:

clearstatcache(true, $path);

Oh, good catch!

jakzal · 2015-10-22T14:58:02Z

@jakzal said in #15813 (comment) that unlink()ing the lock file should clear the statcache, but I am not sure if that is true in Apache prefork MPM setups.

From the php.net/clearstatcache docs:

However unlink() clears the cache automatically.

I don't see how would this be different for apache prefork MPM?

So, my assumption is that the is_file() result is based on the fstat cache and wrong once a process has seen the lock file.

As long as this is just an assumption, and not a confirmed bug, I'm 👎 for this change.

mpdude · 2015-10-22T15:09:14Z

Thoughts:

If you create a "lock" file, run the following script and then remove the file from a different terminal/shell/whatever, the script will be stuck in the loop.

<?php

while (is_file("lock")) {
    print "lock exists...\n";
    sleep(1);
}

Under Apache prefork MPM, I don't see how the statcache could be shared and/or cleared without some advanced IPC messaging. I don't believe PHP does that.
As I said before, trying to file_get_contents when is_file returned true would give "file not found" very often. With this change, I was always able to successfully read the file.

I know that these are just hints – so what kind of "evidence" or confirmation are we looking for?

mpdude · 2015-10-22T15:11:23Z

From the php.net/clearstatcache docs:

However unlink() clears the cache automatically.

It also says:

For instance, if the same file is being checked multiple times within a single script, and that file is in danger of being removed or changed during that script's operation, you may elect to clear the status cache.

Maybe the note regarding unlink() refers only to unlink() calls from within the same script/process.

jakzal · 2015-10-22T18:51:11Z

Maybe the note regarding unlink() refers only to unlink() calls from within the same script/process.

Indeed. Looks like you're right here.

MatTheCat · 2015-11-22T11:04:17Z

@mpdude it's been a month, can you confirm this PR solved the problem?

mpdude · 2015-11-22T13:26:29Z

Haven't seen it for a while, but that's not a 100% guarantee...

fabpot · 2015-11-28T10:49:07Z

Thank you @mpdude.

@jakzal

…k file has been released (mpdude) This PR was squashed before being merged into the 2.3 branch (closes #16312). Discussion ---------- [HttpKernel] clearstatcache() so the Cache sees when a .lck file has been released | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #15813 | License | MIT | Doc PR | n/a I've been trying to debug #15813 and modified the Store in a way to keep unique request IDs in the .lck file. That way, I was hoping to find out which request is blocking and/or if the request is actually still running. It turned out that `is_file()` would claim that a lock file still exists, but a subsequent attempt to read the information from that file returned "file not found" errors. So, my assumption is that the `is_file()` result is based on the fstat cache and wrong once a process has seen the lock file. @jakzal said in #15813 (comment) that `unlink()`ing the lock file should clear the statcache, but I doubt this is true across PHP processes. Commits ------- 982710f [HttpKernel] clearstatcache() so the Cache sees when a .lck file has been released

xabbuh · 2015-11-29T15:55:19Z

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

+        $path = $this->getPath($this->getCacheKey($request).'.lck');
+        clearstatcache(true, $path);
+
+        return is_file($path);


Is the file check absolutely necessary or couldn't we just call file_exists() which doesn't use the stat cache?

The documentation of clearstatcache says file_exists is affected.

@MatTheCat is right here, only the information about non-existent files is not cached:

You should also note that PHP doesn't cache information about non-existent files. So, if you call file_exists() on a file that doesn't exist, it will return FALSE until you create the file. If you create the file, it will return TRUE even if you then delete the file. However unlink() clears the cache automatically.

Thanks, I think I confused that.

This PR was merged into the 2.7 branch. Discussion ---------- [HttpKernel] Use flock() for HttpCache's lock files | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | #16777, #15813 and #16312 are also related | License | MIT | Doc PR | When a PHP process crashes or terminates (maybe the OOM killer kicks in or other bad things ™️ happen) while the `HttpCache` holds a `.lck` file, that lock file may not get `unlink()`ed. The result is that other requests trying to access this cache entry will see a few seconds delay while waiting for the lock; they will eventually continue but send 503 status codes along with the response. The sudden buildup of PHP processes caused by the additional delay may cause further problems (sudden load increase). As `LockHandler` is using `flock()`-based locking, locks should be released by the OS when the PHP process terminates. I wrote this as bugfix against 2.7 because every once in a while I encounter situations (not always reproducible) where `.lock` files are left over and keep the cache locked. Commits ------- 2668edd [HttpKernel] Use flock() for HttpCache's lock files

Add clearstatcache() call before checking cache lock

d374e04

carsonbot added the Status: Needs Review label Oct 22, 2015

mpdude added 2 commits October 22, 2015 12:18

FabBot

d4e92d1

FabBot, again

e4276e7

mpdude mentioned this pull request Oct 22, 2015

[HttpCache] occasionnal 503 error on locking request #15813

Closed

mpdude changed the title ~~Add clearstatcache() call before checking cache lock~~ [HttpKernel] clearstatcache() so the Cache sees when a .lck file has been released Oct 22, 2015

aitboudad reviewed Oct 22, 2015
View reviewed changes

Assumed wrong clearstatcache() signature

7930b75

jakzal added the HttpKernel label Oct 22, 2015

fabpot closed this Nov 28, 2015

mpdude deleted the patch-1 branch November 28, 2015 10:55

xabbuh reviewed Nov 29, 2015
View reviewed changes

This was referenced Nov 30, 2015

Release v2.8.0 #16763

Merged

Release v3.0.0 #16766

Merged

This was referenced Dec 26, 2015

Release v2.3.36 #17141

Merged

Release v2.7.8 #17142

Merged

This was referenced Jul 6, 2016

Use LockHandler to manage HttpCache's lock files #19299

Closed

[HttpKernel] Use flock() for HttpCache's lock files #19300

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpKernel] clearstatcache() so the Cache sees when a .lck file has been released #16312

[HttpKernel] clearstatcache() so the Cache sees when a .lck file has been released #16312

mpdude commented Oct 22, 2015

aitboudad Oct 22, 2015

mpdude Oct 22, 2015

jakzal commented Oct 22, 2015

mpdude commented Oct 22, 2015

mpdude commented Oct 22, 2015

jakzal commented Oct 22, 2015

MatTheCat commented Nov 22, 2015

mpdude commented Nov 22, 2015

fabpot commented Nov 28, 2015

xabbuh Nov 29, 2015

MatTheCat Nov 29, 2015

jakzal Nov 29, 2015

xabbuh Nov 29, 2015

[HttpKernel] clearstatcache() so the Cache sees when a .lck file has been released #16312

[HttpKernel] clearstatcache() so the Cache sees when a .lck file has been released #16312

Conversation

mpdude commented Oct 22, 2015

aitboudad Oct 22, 2015

Choose a reason for hiding this comment

mpdude Oct 22, 2015

Choose a reason for hiding this comment

jakzal commented Oct 22, 2015

mpdude commented Oct 22, 2015

mpdude commented Oct 22, 2015

jakzal commented Oct 22, 2015

MatTheCat commented Nov 22, 2015

mpdude commented Nov 22, 2015

fabpot commented Nov 28, 2015

xabbuh Nov 29, 2015

Choose a reason for hiding this comment

MatTheCat Nov 29, 2015

Choose a reason for hiding this comment

jakzal Nov 29, 2015

Choose a reason for hiding this comment

xabbuh Nov 29, 2015

Choose a reason for hiding this comment