Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Config] Early return for DirectoryResource#21458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Config] Early return for DirectoryResource#21458
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| returnfalse; | ||
| } | ||
| $newestMTime =max($fileMTime,$newestMTime); |
ogizanagiJan 29, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This can be removed, right?
Also, may test directory mtime first (before the foreach), and returnfalse if greater than passed timestamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, removing that breaks the test->isFresh() returns false if an existing file is removed. Yes, though, we could add another condition at the very beginning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Added initial conditional before loop.
ogizanagiJan 29, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let me know if I'm wrong, but I think the test is failing in this case because the implementation and the test case are slightly flawed.
Try:
diff --git a/src/Symfony/Component/Config/Resource/DirectoryResource.php b/src/Symfony/Component/Config/Resource/DirectoryResource.phpindex dcb5e19..bef0b2b 100644--- a/src/Symfony/Component/Config/Resource/DirectoryResource.php+++ b/src/Symfony/Component/Config/Resource/DirectoryResource.php@@ -95,7 +95,7 @@ class DirectoryResource implements SelfCheckingResourceInterface, \Serializable $newestMTime = max($fileMTime, $newestMTime); }- return $newestMTime < $timestamp;+ return $newestMTime <= $timestamp; } public function serialize()
and the test case should fail. However, I'd expect a resource to be considered fresh if its mtime is the exact same as the provided timestamp (on the contrary, you should revised the new conditions to use$timestamp <= $fileMTime when returningfalse).
The following for instance should work:
See patch
diff --git a/src/Symfony/Component/Config/Resource/DirectoryResource.php b/src/Symfony/Component/Config/Resource/DirectoryResource.phpindex dcb5e19..deb40b0 100644--- a/src/Symfony/Component/Config/Resource/DirectoryResource.php+++ b/src/Symfony/Component/Config/Resource/DirectoryResource.php@@ -74,7 +74,10 @@ class DirectoryResource implements SelfCheckingResourceInterface, \Serializable return false; }- $newestMTime = filemtime($this->resource);+ if($timestamp < $newestMTime = filemtime($this->resource)) {+ return false;+ };+ foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($this->resource), \RecursiveIteratorIterator::SELF_FIRST) as $file) { // if regex filtering is enabled only check matching files if ($this->pattern && $file->isFile() && !preg_match($this->pattern, $file->getBasename())) {@@ -88,14 +91,12 @@ class DirectoryResource implements SelfCheckingResourceInterface, \Serializable } // early return if any file mtime is greater than passed timestamp- if (($fileMTime = $file->getMTime()) > $timestamp) {+ if ($timestamp < $fileMTime = $file->getMTime()) { return false; }-- $newestMTime = max($fileMTime, $newestMTime); }- return $newestMTime < $timestamp;+ return true; } public function serialize()diff --git a/src/Symfony/Component/Config/Tests/Resource/DirectoryResourceTest.php b/src/Symfony/Component/Config/Tests/Resource/DirectoryResourceTest.phpindex a22209d..9d8e055 100644--- a/src/Symfony/Component/Config/Tests/Resource/DirectoryResourceTest.php+++ b/src/Symfony/Component/Config/Tests/Resource/DirectoryResourceTest.php@@ -110,8 +110,10 @@ class DirectoryResourceTest extends \PHPUnit_Framework_TestCase public function testIsFreshDeleteFile() { $resource = new DirectoryResource($this->directory);+ $time = time();+ sleep(1); unlink($this->directory.'/tmp.xml');- $this->assertFalse($resource->isFresh(time()), '->isFresh() returns false if an existing file is removed');+ $this->assertFalse($resource->isFresh($time), '->isFresh() returns false if an existing file is removed'); } public function testIsFreshDeleteDirectory()
robfrawleyJan 29, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we consider changing this a BC break, though? That's why I hadn't implemented something similar in my testing leading up to this PR. Assuming the answer is no, I'll happily implement these changes, though@nicolas-grekas@ogizanagi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's rather a bug fix to me.
For reference,FileResource usesfilemtime($this->resource) <= $timestamp too.
robfrawleyJan 29, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Something like this greatly simplifies the code in this PR, and I think may be a viable option (this diff is against this PR), without changing the tests at all:
diff --git a/src/Symfony/Component/Config/Resource/DirectoryResource.php b/src/Symfony/Component/Config/Resource/DirectoryResource.phpindex 9011cfe..501f8a5 100644--- a/src/Symfony/Component/Config/Resource/DirectoryResource.php+++ b/src/Symfony/Component/Config/Resource/DirectoryResource.php@@ -68,7 +68,7 @@ class DirectoryResource implements ResourceInterface, \Serializable return false; }- if ($timestamp < $newestMTime = filemtime($this->resource)) {+ if ($timestamp <= filemtime($this->resource)) { return false; }@@ -85,14 +85,12 @@ class DirectoryResource implements ResourceInterface, \Serializable } // early return if a file's mtime exceeds the passed timestamp- if ($timestamp < $fileMTime = $file->getMTime()) {+ if ($timestamp <= $file->getMTime()) { return false; }-- $newestMTime = max($fileMTime, $newestMTime); }- return $newestMTime < $timestamp;+ return true; } public function serialize()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's see what the maintainers say; changes pushed.
robfrawleyJan 29, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I too think that the same timestamp should be considered fresh. Can you clarify why the behavior exists such that a matching timestamp isnot fresh@nicolas-grekas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be fresh with same timestamp to me also - same as in FileResource
cab5763 tofc1726fComparefc1726f tod9701bdCompare| } | ||
| $newestMTime =filemtime($this->resource); | ||
| if (($newestMTime =filemtime($this->resource)) >$timestamp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can swap for a<, and remove the brackets as done usually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same below btw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 done
d9701bd to97bda48Compare
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 with minor typo
| } | ||
| $newestMTime =max($file->getMTime(),$newestMTime); | ||
| // early return if a file's mtime is exceeds than passed timestamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is exceeds =>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed ;-)
8fabc4f to5eac053Compare5eac053 to96107e2Compare
xabbuh left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
| } | ||
| $newestMTime =filemtime($this->resource); | ||
| if ($timestamp <=filemtime($this->resource)) { |
nicolas-grekasJan 30, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this needs to be turned to a<: the "equals" case should not return false, as in eg FileResource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It returnedfalse before. Do we consider this behaviour change a bug fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Check second commit (can squash if its good)
| $newestMTime =max($file->getMTime(),$newestMTime); | ||
| // early return if a file's mtime exceeds the passed timestamp | ||
| if ($timestamp <=$file->getMTime()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
<
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Check second commit (can squash if its good)
| sleep(1); | ||
| unlink($this->directory.'/tmp.xml'); | ||
| $this->assertFalse($resource->isFresh(time()),'->isFresh() returns false if an existing file is removed'); | ||
| $this->assertFalse($resource->isFresh($time),'->isFresh() returns false if an existing file is removed'); |
robfrawleyJan 30, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicolas-grekas This fails with the "bug fix" tonot returnfalse when timestamp are the same (unlike the original implementation). This test change fixes failures originating from this behavioral change...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't we move this test into thetime-sensitive group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch; I didn't know about that convention. I only see that group at the class-level. Is it okay to use it at the test-level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, that should work the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Adding that group causes the test to fail. What does that group do and why would it result in it failing?
robfrawleyJan 30, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@xabbuh We can't use that because the internal implementation only relates to the PHP interpreter. This relies on thefilesystem and is external of the interpreter.
An added bonus of using the ClockMock class is that time passes instantly. Using PHP's sleep(10) will make your test wait for 10 actual seconds (more or less). In contrast, the ClockMock class advances the internal clock the given number of seconds without actually waiting that time, so your test will execute 10 seconds faster.
http://symfony.com/blog/new-in-symfony-2-8-clock-mocking-and-time-sensitive-tests
fabpot commentedFeb 12, 2017
Thank you@robfrawley. |
This PR was squashed before being merged into the 2.7 branch (closes#21458).Discussion----------[Config] Early return for DirectoryResource| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | sure?| BC breaks? | no| Deprecations? | no| Tests pass? | no| Fixed tickets || Related PRs |#21440| License | MIT| Doc PR | n/aAlternate PR that implements an early return for `DirectoryResource` to increase the speed on large file sets. We can never return early with `true` without checking all assets within the resource, as the aforementioned referenced PR did; hence this PR takes the counter approach and returns `false` early where appropriate._Conversation about possible bug at#21458 (comment)Commits-------d5746ec fix directory resource considers same timestamp not fresh96107e2 return false early from directory resource
Uh oh!
There was an error while loading.Please reload this page.
Alternate PR that implements an early return for
DirectoryResourceto increase the speed on large file sets. We can never return early withtruewithout checking all assets within the resource, as the aforementioned referenced PR did; hence this PR takes the counter approach and returnsfalseearly where appropriate.Conversation about possible bug at#21458 (comment).