Remove empty locked cache file if callback function terminate process #15592

Merged
merged 2 commits into from Apr 28, 2017

Conversation

Projects
None yet
@csthomas
Contributor

csthomas commented Apr 26, 2017

Pull Request for Issue #15544

Summary of Changes

Remove empty locked file at shutdown script if cache callback function terminate process on 404.

This is my second PR (previous #15558)

Reason

To save a cache file joomla requires lock method.
To create a lock for cache joomla has to create a file in the cache folder.
If a view method raise an error then cache process is hung up and the file can not be deleted.
The file is left empty.
Next request could find such file and display it as empty article.

Testing Instructions

Take a look at the issue.

Expected result

Error 404

Actual result

Blank component.

Documentation Changes Required

No

libraries/joomla/cache/storage/file.php
@@ -420,6 +445,11 @@ protected function _checkExpire($id, $group)
return false;
}
+ if (filesize($path) == 0)

This comment has been minimized.

@Quy

Quy Apr 26, 2017

Contributor

Do you want this line to be consistent with line 63 912661f ?
if (@filesize($path) === 0)

@Quy

Quy Apr 26, 2017

Contributor

Do you want this line to be consistent with line 63 912661f ?
if (@filesize($path) === 0)

This comment has been minimized.

@csthomas

csthomas Apr 26, 2017

Contributor

No,
Line 448: I can be almost sure that this file exists. But if not then it also should return false. Maybe I should add '@'

if (filesize($path) == 0)
{
	return false;

Line 63: If file won't exists then do not try to delete a file which does not exists.

if (@filesize($path) === 0)
{
	@unlink($path);

filesize('/not_existed_file') will return false and will generate PHP Warning.

@csthomas

csthomas Apr 26, 2017

Contributor

No,
Line 448: I can be almost sure that this file exists. But if not then it also should return false. Maybe I should add '@'

if (filesize($path) == 0)
{
	return false;

Line 63: If file won't exists then do not try to delete a file which does not exists.

if (@filesize($path) === 0)
{
	@unlink($path);

filesize('/not_existed_file') will return false and will generate PHP Warning.

This comment has been minimized.

@Quy

Quy Apr 26, 2017

Contributor

Yes I was only referring to line 63 to making it the same with @ and ===.

@Quy

Quy Apr 26, 2017

Contributor

Yes I was only referring to line 63 to making it the same with @ and ===.

@@ -445,7 +446,8 @@ protected function _checkExpire($id, $group)
return false;
}
- if (filesize($path) == 0)
+ // If now the file does not exist then return false too.
+ if (@filesize($path) == 0)

This comment has been minimized.

@Quy

Quy Apr 26, 2017

Contributor

Make it ===??

@Quy

Quy Apr 26, 2017

Contributor

Make it ===??

This comment has been minimized.

@csthomas

csthomas Apr 26, 2017

Contributor

If I do this then if file won't exist then it goes to return true (which will be wrong)

@csthomas

csthomas Apr 26, 2017

Contributor

If I do this then if file won't exist then it goes to return true (which will be wrong)

This comment has been minimized.

@PhilETaylor

PhilETaylor May 4, 2017

Contributor

would be better to check the file exists and then if it exists then check its size - instead of using the @ error suppressor which should not be used without a very very good case for its use.

@PhilETaylor

PhilETaylor May 4, 2017

Contributor

would be better to check the file exists and then if it exists then check its size - instead of using the @ error suppressor which should not be used without a very very good case for its use.

This comment has been minimized.

@csthomas

csthomas May 4, 2017

Contributor

Then between file_exists() and filemsize() will be a little time space when another process can delete the file and then you get PHP Warning. I know this is no often but this is the reason of '@'.

@csthomas

csthomas May 4, 2017

Contributor

Then between file_exists() and filemsize() will be a little time space when another process can delete the file and then you get PHP Warning. I know this is no often but this is the reason of '@'.

This comment has been minimized.

@PhilETaylor

PhilETaylor May 4, 2017

Contributor

I believe that EVERY use of @Supressor should be documented - not for you, but for the poor sod debugging it in a years time.

@PhilETaylor

PhilETaylor May 4, 2017

Contributor

I believe that EVERY use of @Supressor should be documented - not for you, but for the poor sod debugging it in a years time.

This comment has been minimized.

@mbabker

mbabker May 4, 2017

Member

The use is fine here, just as Phil pointed out, it'd be better to explain why we need it here (very common scenario honestly).

@mbabker

mbabker May 4, 2017

Member

The use is fine here, just as Phil pointed out, it'd be better to explain why we need it here (very common scenario honestly).

This comment has been minimized.

@csthomas

csthomas May 4, 2017

Contributor

OK, feel free to add this.

@csthomas

csthomas May 4, 2017

Contributor

OK, feel free to add this.

This comment has been minimized.

@flyingwombats

This comment has been minimized.

Show comment
Hide comment
@flyingwombats

flyingwombats Apr 27, 2017

Tested this patch under the conditions laid out in the original issue and I can confirm it now brings back a proper 404 when before it was not.

Tested this patch under the conditions laid out in the original issue and I can confirm it now brings back a proper 404 when before it was not.

@csthomas

This comment has been minimized.

Show comment
Hide comment
@csthomas

csthomas Apr 27, 2017

Contributor

@flyingwombats Thanks.
Can you go to https://issues.joomla.org/tracker/joomla-cms/15592 and click on button "Test this" and mark test as success.

Contributor

csthomas commented Apr 27, 2017

@flyingwombats Thanks.
Can you go to https://issues.joomla.org/tracker/joomla-cms/15592 and click on button "Test this" and mark test as success.

@flyingwombats

This comment has been minimized.

Show comment
Hide comment
@flyingwombats

flyingwombats Apr 27, 2017

I have tested this item successfully on fb600c2

404 returns correctly


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15592.

I have tested this item successfully on fb600c2

404 returns correctly


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15592.

@OctavianC

This comment has been minimized.

Show comment
Hide comment
@OctavianC

OctavianC Apr 28, 2017

Contributor

I have tested this item successfully on fb600c2

Tested and solves the issue, I can confirm it fixes #15647 as well.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15592.

Contributor

OctavianC commented Apr 28, 2017

I have tested this item successfully on fb600c2

Tested and solves the issue, I can confirm it fixes #15647 as well.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15592.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Apr 28, 2017

RTC after two successful tests.

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Apr 28, 2017

@wilsonge wilsonge merged commit 85e4744 into joomla:staging Apr 28, 2017

4 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Apr 28, 2017

@wilsonge wilsonge modified the milestones: Joomla 4.0, Joomla 3.7.1 Apr 28, 2017

@madesign

This comment has been minimized.

Show comment
Hide comment
@madesign

madesign Apr 30, 2017

Sorry, i do not understand quit well what i have to do to fix the issue.

Sorry, i do not understand quit well what i have to do to fix the issue.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment

rdeutz added a commit to joomlajenkins/joomla-cms that referenced this pull request May 1, 2017

Remove empty locked cache file if callback function terminate process (
…#15592)

* Remove empty locked cache file if callback function terminate process

* Add comments and @

@krogias krogias referenced this pull request in itsam/imc May 2, 2017

Closed

White screen in Joomla 3.7 #65

@tampe125

This comment has been minimized.

Show comment
Hide comment
@tampe125

tampe125 May 4, 2017

Contributor

Why are we creating files using the c flag? Isn't the r good enough?
For your information, the function register_shutdown_function is not 100% reliable

Contributor

tampe125 commented May 4, 2017

Why are we creating files using the c flag? Isn't the r good enough?
For your information, the function register_shutdown_function is not 100% reliable

@csthomas

This comment has been minimized.

Show comment
Hide comment
@csthomas

csthomas May 4, 2017

Contributor

Why are we creating files using the c flag? Isn't the r good enough?

r does not create a file. No file - no lock.
I have to create a lockfile before I will create a cache.

For your information, the function register_shutdown_function is not 100% reliable

Function register_shutdown_function should be called in almost 100% cases, except fatal errors.
I know it looks weird but callback function can terminate process by exit(0).

May be the best way would be to not block other processes to create a cache data but only block on save the data to cache file.

Old version does not create a lock for not existed cache files.
There was an error because for example: 10 processes can create the same cache file in the same time and then they override the file a few times.

Contributor

csthomas commented May 4, 2017

Why are we creating files using the c flag? Isn't the r good enough?

r does not create a file. No file - no lock.
I have to create a lockfile before I will create a cache.

For your information, the function register_shutdown_function is not 100% reliable

Function register_shutdown_function should be called in almost 100% cases, except fatal errors.
I know it looks weird but callback function can terminate process by exit(0).

May be the best way would be to not block other processes to create a cache data but only block on save the data to cache file.

Old version does not create a lock for not existed cache files.
There was an error because for example: 10 processes can create the same cache file in the same time and then they override the file a few times.

@waveywhite

This comment has been minimized.

Show comment
Hide comment
@waveywhite

waveywhite May 5, 2017

Contributor

I've just tried this fix on PHP 5.5.9 and it is now not saving any page cache files. I was suffering from the "zero length cache file" problem for pages which the page cache plugin ignored. This fix is now removing all cache files.

We're using a custom page cache plugin based on the Joomla one. It was working fine on 3.6.x. I'll also check that there aren't any changes to the 3.7 page cache plugin we should be considering.

Contributor

waveywhite commented May 5, 2017

I've just tried this fix on PHP 5.5.9 and it is now not saving any page cache files. I was suffering from the "zero length cache file" problem for pages which the page cache plugin ignored. This fix is now removing all cache files.

We're using a custom page cache plugin based on the Joomla one. It was working fine on 3.6.x. I'll also check that there aren't any changes to the 3.7 page cache plugin we should be considering.

@csthomas

This comment has been minimized.

Show comment
Hide comment
@csthomas

csthomas May 5, 2017

Contributor

IMO your problem is somewhere else.
Take a look at https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/cache/cache.php#L251-L254

Can you test on memcache(-d) or redis?

Contributor

csthomas commented May 5, 2017

IMO your problem is somewhere else.
Take a look at https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/cache/cache.php#L251-L254

Can you test on memcache(-d) or redis?

@csthomas

This comment has been minimized.

Show comment
Hide comment
@csthomas

csthomas May 5, 2017

Contributor

I think about redesign it once again and remove cache locking from outside get and store methods.

Contributor

csthomas commented May 5, 2017

I think about redesign it once again and remove cache locking from outside get and store methods.

@waveywhite

This comment has been minimized.

Show comment
Hide comment
@waveywhite

waveywhite May 5, 2017

Contributor

Wow, that was a headache. Seriously, can someone put memcached configurations into docs.joomla.org?

Anyway, I can confirm that there are no problems with memcache, it is caching the correct locations and has no problems on locations where our plugin skips caching.

Contributor

waveywhite commented May 5, 2017

Wow, that was a headache. Seriously, can someone put memcached configurations into docs.joomla.org?

Anyway, I can confirm that there are no problems with memcache, it is caching the correct locations and has no problems on locations where our plugin skips caching.

@csthomas

This comment has been minimized.

Show comment
Hide comment
@csthomas

csthomas May 5, 2017

Contributor

Can you put fflush($_fileopen); after $result = @fwrite($_fileopen, $data, $length); line 216 in file libraries/joomla/cache/storage/file.php and check if this resolve your problem?

Contributor

csthomas commented May 5, 2017

Can you put fflush($_fileopen); after $result = @fwrite($_fileopen, $data, $length); line 216 in file libraries/joomla/cache/storage/file.php and check if this resolve your problem?

@waveywhite

This comment has been minimized.

Show comment
Hide comment
@waveywhite

waveywhite May 5, 2017

Contributor

OK. Do you mean in the patched file.php or the Joomla 3.7.0 version?

Contributor

waveywhite commented May 5, 2017

OK. Do you mean in the patched file.php or the Joomla 3.7.0 version?

@csthomas

This comment has been minimized.

Show comment
Hide comment
@csthomas

csthomas May 5, 2017

Contributor

In the patched file

Contributor

csthomas commented May 5, 2017

In the patched file

@csthomas

This comment has been minimized.

Show comment
Hide comment
@csthomas

csthomas May 5, 2017

Contributor

You can also add a few lines below fclose($_fileopen) another function clearstatcache();

Contributor

csthomas commented May 5, 2017

You can also add a few lines below fclose($_fileopen) another function clearstatcache();

@waveywhite

This comment has been minimized.

Show comment
Hide comment
@waveywhite

waveywhite May 5, 2017

Contributor

I've got the following:

                if ($_fileopen)
                {
                        $length = strlen($data);
                        $result = @fwrite($_fileopen, $data, $length);
                        fflush($_fileopen);

                        if ($close)
                        {
                                @fclose($_fileopen);
                        }
                        clearstatcache();

                        return $result === $length;
                }

But it's not helping. There are still no files ending up in cache
On a different server (same site, same OS - Ubuntu 14.04) it's working OK. Could we have a race condition?

Contributor

waveywhite commented May 5, 2017

I've got the following:

                if ($_fileopen)
                {
                        $length = strlen($data);
                        $result = @fwrite($_fileopen, $data, $length);
                        fflush($_fileopen);

                        if ($close)
                        {
                                @fclose($_fileopen);
                        }
                        clearstatcache();

                        return $result === $length;
                }

But it's not helping. There are still no files ending up in cache
On a different server (same site, same OS - Ubuntu 14.04) it's working OK. Could we have a race condition?

@csthomas

This comment has been minimized.

Show comment
Hide comment
@csthomas

csthomas May 5, 2017

Contributor

Example is OK. Then I have no more ideas now.
One php process should create some cache files. Do you have correct permission and free space on disk?

Contributor

csthomas commented May 5, 2017

Example is OK. Then I have no more ideas now.
One php process should create some cache files. Do you have correct permission and free space on disk?

@waveywhite

This comment has been minimized.

Show comment
Hide comment
@waveywhite

waveywhite May 5, 2017

Contributor

OK, it's working now. It looks like it was trouble at my end which occurred at the exact same time I applied this patch.

It seems that the cache/page folder was recreated in some way, probably via an rsync action, while my terminal's CWD was in that folder. So, when I ls -l I see no cache files. However, several hours later I change directory and voilà, all the files are in the 'new' cache/page folder and it was working all along.

Sorry to waste your time and thank you for your efforts to sort this out.

Contributor

waveywhite commented May 5, 2017

OK, it's working now. It looks like it was trouble at my end which occurred at the exact same time I applied this patch.

It seems that the cache/page folder was recreated in some way, probably via an rsync action, while my terminal's CWD was in that folder. So, when I ls -l I see no cache files. However, several hours later I change directory and voilà, all the files are in the 'new' cache/page folder and it was working all along.

Sorry to waste your time and thank you for your efforts to sort this out.

@csthomas csthomas deleted the csthomas:fix37cachefile2 branch May 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment