-
Notifications
You must be signed in to change notification settings - Fork 78
Fix error alert on prod objectfs #702
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: MOODLE_404_STABLE
Are you sure you want to change the base?
Conversation
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.
Thanks @duyhuynhdev
This commit is fixing a handful of different issues, some of which are mostly unrelated. Can we please split this into separate commits with descriptive commit messages?
Something like:
- Ensure locks are released properly
- Add fallback check for directories created between calls
As we don't have an issue raised for these it would also be worth explaining why these are a problem.
| // Permission trouble. | ||
| throw new file_exception('storedfilecannotcreatefiledirs'); | ||
| } | ||
| if (!is_dir($localdirpath) && !@mkdir($localdirpath, $this->dirpermissions, true) && !is_dir($localdirpath)) { |
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 really want to suppress mkdir errors? My gut feel is this should still logged, especially when an exception is thrown. Would also recommend leaving a comment as to why we have the second check.
Also do we know what is creating a directory between calls? Is this something making a directory without a lock or a lock not working? This looks correct to me, but I'm a bit concerned if there can be other conflicts later then the fix could potentially be worse than the current issue.
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.
@bwalkerl , I did some research and found this is the common practice for this scenario. If you search for @mkdir, you’ll see the is_dir → @mkdir → is_dir pattern used widely, including in core. That’s why I used @, though I agree it’s risky. On PHP < 8.0 it can suppress critical errors. So I will remove it in the next commit.
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.
More than anything the reason we want the warnings is because we don't know what else is creating them, and that's the biggest worry, especially since this function should have a lock.
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.
get_contentdir_from_hash returns the comon hash prefix, so two processes could be working on 2 files with their own locks but by chance still have a common prefix and thats fine
47d4703 to
4b78f63
Compare
|
Just muddying the waters slightly at this one got merged which fixes some of the same bits: @duyhuynhdev will rebase this and remove the duplicate bits and we'll re-review shortly |
3e30754 to
b94830a
Compare
Fix error alert on prod objectfs