Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] prevent cache:clear creating too long paths#16829
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Tobion commentedDec 4, 2015
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #15547#16783 |
| License | MIT |
| Doc PR | - |
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 was really dumb. it renamed the folder, just to remove it then.
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.
Isn't the purpose of that to provide a clean cache dir faster? Since renaming takes less time than removing.
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 you have some evidence for this?
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 is obviously heavily platform dependent, so its difficult to provide good evidence.
As a comparison on a high / low resource system: (Cache size: 11.2MB, 772 files, 512 directories)
| 50 iterations | Rename | Remove |
|---|---|---|
| high | ~3ms | ~687ms |
| low | ~4ms | ~3708ms |
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'd the purpose of this line is to have an atomic clearing of the cache. If I'm not wrong, we should keep it (and add a comment about it)
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 depends on the filesystem you use. Normally removing a directory only involves removing a reference it to. Also I don't think it matters at the end and I could not find any information that this was done on purpose in git.
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.
Ok I'll revert that. From what I read, moving a directory is more likely to be atomic than deleting. Most filesystems implement moving a directory on the same partition atomically.
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.
reverted
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 was done twice
Tobion commentedDec 7, 2015
ping @symfony/deciders this fix is important to merge as the bug is currently causing problems for all windows users running symfony. Together withdoctrine/cache#107 it is gone. |
fabpot commentedDec 7, 2015
👍 |
xabbuh commentedDec 7, 2015
👍 Status: Reviewed |
…ths (Tobion)This PR was merged into the 2.3 branch.Discussion----------[FrameworkBundle] prevent cache:clear creating too long paths| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#15547#16783| License | MIT| Doc PR | -Commits-------6e279c5 [FrameworkBundle] prevent cache:clear creating too long paths