| View previous topic :: View next topic |
| Author |
Message |
maarchewa
Smarty n00b
Joined: 22 Aug 2011
Posts: 3
|
 Posted: Mon Aug 22, 2011 12:24 pm Post subject: getCachedTimestamp - cache bug |
 |
|
Smarty version: 3.0.6
file: smarty_internal_cacheresource_file.php
function: getCachedTimestamp
details:
current function code:
| Code: |
public function getCachedTimestamp($_template)
{
// return @filemtime ($_template->getCachedFilepath());
return ($_template->getCachedFilepath() && file_exists($_template->getCachedFilepath())) ? filemtime($_template->getCachedFilepath()) : false ;
}
|
The problem is that file can be deleted between file existance check and file modification time check. In my web app (with cache enabled) about 10% of requests lands with E_WARNING notification (generated by filemtime function).
When I terminate application on E_WARNING notification I can realize that checked file DOES NOT exist (even if existance was checked).
Please add error suppression to the filemtime function (code that was commented out was correct). |
|
| Back to top |
|
rodneyrehm
Administrator
Joined: 30 Mar 2007
Posts: 674
Location: Germany, border to Switzerland
|
| Posted: Mon Aug 22, 2011 4:41 pm Post subject: Re: getCachedTimestamp - cache bug |
|
|
| maarchewa wrote: | | Please add error suppression to the filemtime function (code that was commented out was correct). |
Smarty 3.1 should already solve the issue
_________________
Twitter |
|
| Back to top |
|
maarchewa
Smarty n00b
Joined: 22 Aug 2011
Posts: 3
|
| Posted: Tue Aug 23, 2011 11:13 am Post subject: |
|
|
problem still exists in version 3.1RC1
E_WARNING is generated in smarty_internal_cacheresource_file.php in process function:
| Code: | smarty_internal_cacheresource_file.php:78 - 2:
include(xxx) [<a href='function.include'>function.include</a>]: failed to open stream: No such file or directory |
Proposed solution:
- smarty_internal_cacheresource_file.php:78 function process, from
| Code: | | return include $_template->cached->filepath; |
to
| Code: | | return @include $_template->cached->filepath; |
- smarty_cacheresource.php:241 function __constructor, from
| Code: |
$handler->process($_template)
$this->processed = true;
|
to
| Code: |
if($handler->process($_template) === false)
$this->valid = false;
else {
$this->processed = true;
}
|
Please let me know if proposed solution is correct - no notification is generated and everything seems to be fine but I need to be sure  |
|
| Back to top |
|
rodneyrehm
Administrator
Joined: 30 Mar 2007
Posts: 674
Location: Germany, border to Switzerland
|
| Posted: Tue Aug 23, 2011 11:42 am Post subject: |
|
|
Ah, I see. It's not fixed for the filesystem implementation.
Your proposed fix is bad, as it doesn't "fix" anything, just masks an error. You see, the include fails, thus the cache cannot be read, thus smarty would output an empty page. NOT what you expect to happen.
What we have to do here is making sure the cached content is read to memory the instant we get a timestamp (cache hit). Thus the running process must not access the cache a second time.
The main problem will be having to use eval() instead of include. eval() is slow as resulting op-code is currently not cachable by any means (APC and co). I'll discuss the issue with Uwe once he's back.
_________________
Twitter |
|
| Back to top |
|
U.Tews
Administrator
Joined: 22 Nov 2006
Posts: 5068
Location: Hamburg / Germany
|
| Posted: Fri Aug 26, 2011 8:31 pm Post subject: |
|
|
The proposed solution for 3.1 alone did not cover all possible cases.
The complete fix is in the Smarty_3_1_DEV SVN branch now. |
|
| Back to top |
|
pitbull82
Smarty Rookie
Joined: 01 Feb 2006
Posts: 18
Location: Poland
|
| Posted: Tue Sep 20, 2011 11:15 am Post subject: |
|
|
Hello
Is this fix in distribution directory in SVN? If so, I think that there's still a problem with caching.
I got notices:
PHP Warning (2): filemtime() [<a href='function.filemtime'>function.filemtime</a>]: stat failed for D:\webmaster\www_wamp\xxx\app\View-Compiled\pl\c8\bf\f2\c8bff2b2f6d7bd060b1a0aba9fc49589387b6345.file.ranking.tpl.php in D:\webmaster\www_wamp\xxx\libs\templates31dev\sysplugins\smarty_resource.php:607
or
PHP Warning (2): unlink(D:\webmaster\www_wamp\xxx\app\View-Compiled\pl\c8\bf\f2\c8bff2b2f6d7bd060b1a0aba9fc49589387b6345.file.ranking.tpl.php) [<a href='function.unlink'>function.unlink</a>]: No such file or directory in D:\webmaster\www_wamp\xxx\libs\templates31dev\sysplugins\smarty_internal_write_file.php:49
The problem occurs on windows and also on unix based systems - please fix it quickly - it's very hard to find other problems in error logs  |
|
| Back to top |
|
rodneyrehm
Administrator
Joined: 30 Mar 2007
Posts: 674
Location: Germany, border to Switzerland
|
| Posted: Tue Sep 20, 2011 11:36 am Post subject: |
|
|
| pitbull82 wrote: | | Is this fix in distribution directory in SVN? If so, I think that there's still a problem with caching. |
There is no problem with caching. The lines of code you refer to will not output any warnings, unless you've declared your own error_handler - and therein failed to respect the error_level.
Please scan your code for use of http://php.net/set_error_handler and fix your handler to ignore | Code: | | $errno && ($error & error_reporting()) |
.
_________________
Twitter |
|
| Back to top |
|
Setor
Smarty Rookie
Joined: 12 Nov 2009
Posts: 5
|
| Posted: Tue Sep 20, 2011 1:14 pm Post subject: |
|
|
Smarty 3.1
| Code: |
file: smarty_resource.php
line: 599
$compiled->timestamp = @filemtime($compiled->filepath);
$compiled->exists = !!$compiled->timestamp;
|
Warning: filemtime(): stat failed for ..
| Code: |
file: smarty_internal_file_write.php
line: 49
unlink($_filepath);
|
Oh my god... Tricks with error_reporting is very bad style!
Good bye Smarty, hello Twig! |
|
| Back to top |
|
rodneyrehm
Administrator
Joined: 30 Mar 2007
Posts: 674
Location: Germany, border to Switzerland
|
| Posted: Tue Sep 20, 2011 1:21 pm Post subject: |
|
|
| Setor wrote: | | Oh my god... Tricks with error_reporting is very bad style! |
I don't disagree. But in PHP's current state there is no other way. There is no way to make certain functions atomic. Thus there is no way to make sure that between a file_exists() and a filemtime() call some other process did not delete the file. As you can infer from a valid mtime that the file exists, it's only logic to avoid the file_exists() call altogether.
I havent looked too closely at Twig (or any other engine, for that matter). I don't believe they solved this (if at all) any other way.
_________________
Twitter |
|
| Back to top |
|
Setor
Smarty Rookie
Joined: 12 Nov 2009
Posts: 5
|
| Posted: Tue Sep 20, 2011 1:50 pm Post subject: |
|
|
| I understand you, but i catch all warnings and notices. And many frameworks have custom error handlers. My general rule is: Notice/Warning = fatal error. |
|
| Back to top |
|
rodneyrehm
Administrator
Joined: 30 Mar 2007
Posts: 674
Location: Germany, border to Switzerland
|
| Posted: Tue Sep 20, 2011 2:02 pm Post subject: |
|
|
| Setor wrote: | | I understand you, but i catch all warnings and notices. And many frameworks have custom error handlers. My general rule is: Notice/Warning = fatal error. |
I don't know a single library that would comply with your statement. There are simple things like @mkdir($path, $mode, true), @unlink($path), … that just don't _need_ the file_exists() checks up front. Then there's the race condition problem mentioned above. There are situations where you really just want to, or even need to, ignore a warning.
Well, anyways. We use error suppression at points we feel we _need_ them. We know what we're doing and usually have a pretty good reason for doing so.
(yes, we even test performance differences of @, error_reporting() and set_error_handler() before going with any of them.)
_________________
Twitter |
|
| Back to top |
|
pitbull82
Smarty Rookie
Joined: 01 Feb 2006
Posts: 18
Location: Poland
|
| Posted: Tue Sep 20, 2011 4:36 pm Post subject: |
|
|
Yes, I use error handlers but in my application I want to have all errors, notices and warnings in my log because they are potential problems.
My point of view is that any library I use shouldn't generate any warnings and notices when it's working fine. It should be done be library to handle them properly.
In addition I set my errors to
| Code: |
ini_set('display_errors','0');
ini_set("log_errors" , "1");
ini_set("error_log" , 'file.txt');
error_reporting(0);
|
so using
| Code: | | $errno && ($error & error_reporting()) |
makes that I won't have any errors in my log. |
|
| Back to top |
|
rodneyrehm
Administrator
Joined: 30 Mar 2007
Posts: 674
Location: Germany, border to Switzerland
|
| Posted: Tue Sep 20, 2011 5:23 pm Post subject: |
|
|
we could wrap the fetch() method: | Code: | function fetch() {
// mute errors that are masked by @silence or error_reporting level
// but have all other errors bubble through to the next handler
set_error_handler(function($errno){
return $errno && $errno & error_reporting();
});
// execute template stuff
parent::fetch();
// restore previous handler
restore_error_handler();
} |
with that in place, your error handler is not invoked when we don't want it to be. This being an overhead for most people, I'd make it an opt-in feature.
Something you could live with?
_________________
Twitter |
|
| Back to top |
|
rodneyrehm
Administrator
Joined: 30 Mar 2007
Posts: 674
Location: Germany, border to Switzerland
|
| Posted: Wed Sep 21, 2011 10:22 pm Post subject: |
|
|
I implemented this error muting and committed it to trunk. For those of us not using broken error handlers, it can be disabled with | Code: | | Smarty::$error_muting = false; |
_________________
Twitter |
|
| Back to top |
|
pitbull82
Smarty Rookie
Joined: 01 Feb 2006
Posts: 18
Location: Poland
|
| Posted: Thu Sep 22, 2011 9:24 pm Post subject: |
|
|
| It seems that's what I excpected - no warnings generated by Smarty - now it's much easier to read log and find bugs in your application (I've just found one ). Thanks! |
|
| Back to top |
|
|
